Skip to content

Refactor type hints and clean up code#432

Draft
hunterhogan wants to merge 8 commits into
fonttools:mainfrom
hunterhogan:typing
Draft

Refactor type hints and clean up code#432
hunterhogan wants to merge 8 commits into
fonttools:mainfrom
hunterhogan:typing

Conversation

@hunterhogan

@hunterhogan hunterhogan commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Update type hints to use type instead of Type, remove unnecessary checks, and fix typos throughout the codebase. Enhance type annotations for better clarity and maintainability. A continuation of #423.

Comment thread tests/serde/test_json.py Outdated
Comment thread src/ufoLib2/objects/font.py
Comment thread src/ufoLib2/objects/lib.py Outdated

@hunterhogan hunterhogan left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Override the Black profile magic trailing comma for short lines in import statements with split_on_trailing_comma = false.

If you want to disable the magic trailing comma in Black, so all short lines, not just import statements, would be condensed, add skip-magic-trailing-comma = true to pyproject.toml.

@hunterhogan hunterhogan marked this pull request as draft June 28, 2026 19:53
@hunterhogan

Copy link
Copy Markdown
Contributor Author

I created a scaffold for a recursive PlistEncodable TypeAlias and a recursive PlistEncodable TypeVar. But to fully implement the typing and logic, I need your help because I don't know enough about plist or cattrs.

I had to temporarily code the scaffold in my idiosyncratic style so that I could "see" the flow.

I created 3 TypeAlias and 3 TypeVar because it's not yet clear which objects we should use, but we almost certainly will not need all of them.

Note that bytes is not included in those 6 objects. We might want to create something like

PlistScalarBytesBytes: TypeAlias = bool | bytes | datetime | float | int | str
PlistContainerBytes: TypeAlias = (Mapping[str, PlistScalarBytes] | list[PlistScalarBytes] | tuple[PlistScalarBytes, ...])
PlistEncodableBytes: TypeAlias = (Mapping[str, PlistContainerBytes] | list[PlistContainerBytes] | tuple[PlistContainerBytes, ...])

At any rate, if you can apply the concepts to class Lib, _structure, and _structure_data_inplace, at least in part, that will help me understand what is supposed to happen, and I'll be able to write code that matches the codebase.

@hunterhogan

Copy link
Copy Markdown
Contributor Author

There's only a 0.001% chance you want this, but...

	"形": {
		"prefix": "xing",
		"body": "",
		"description": "形 (xíng): type parameter"
	},

identifiers.code-snippets.json

Oh, crap. I just realized the character will probably break your monospacing. Sorry. This is what I use. https://github.com/hunterhogan/Integrated_code_fire_font

@madig

madig commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

I'll need some time to look into recursive types. What is your intention here?

@hunterhogan

hunterhogan commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

I'll need some time to look into recursive types. What is your intention here?

I observed this commented-out code.

# unfortunately mypy is not smart enough to support recursive types like plist...
# PlistEncodable = Union[
# bool,
# bytes,
# datetime,
# float,
# int,
# str,
# Mapping[str, PlistEncodable],
# Sequence[PlistEncodable],
# ]

I formed the belief that it was indicative of the types, containers, and nesting in unstructure. Furthermore, I formed the beliefs that it would be useful in structure and that someone wanted a recursive type in that module.

So, I created a variety of extremely modular, extensible components. Then I implemented overloads and annotations on unstructure that do not trigger type-checker diagnostics to demonstrate that it is syntactically possible to implement recursive types with the components I created.

Someone lamented

unfortunately mypy is not smart enough to support recursive types like plist...

I hope the components will help address that comment.

I guess I am saying: I can't see the "big picture," so here are a bunch of options. Let's work together to select a good system and fine-tune it.

@madig

madig commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Okay, I remember writing this. It seems that mypy now supports recursive types, this works:

from __future__ import annotations

from collections.abc import Mapping, Sequence
from datetime import datetime
from typing import TypeAlias

PlistEncodable: TypeAlias = (
    bool
    | bytes
    | datetime
    | float
    | int
    | str
    | Mapping[str, "PlistEncodable"]
    | Sequence["PlistEncodable"]
)


def hello(a: PlistEncodable) -> None:
    pass


hello(True)
hello(b"abc")
hello(datetime(2026, 1, 1))
hello(1.234)
hello(9)
hello("test")
hello([True, b"abc", datetime(2026, 1, 1), 1.234, 9, "test"])
hello(
    {
        "key": False,
        "hello": b"world",
        "nested": [
            True,
            b"abc",
            datetime(2026, 1, 1),
            1.234,
            9,
            "test",
            {"abc": {"deeper": True}},
        ],
    }
)
❯ uvx mypy --strict test.py
Success: no issues found in 1 source file

I'm using TypeAlias here because the type keyword is only supported by 3.12+.

Can you please retry with this definition? Having one type is easier.

@hunterhogan

Copy link
Copy Markdown
Contributor Author

Excellent, PlistEncodable: TypeAlias is much easier to use.

I moved the module in the right direction. Now, I need your help with some nitty-gritty details and with the complex interaction between serde, cattrs.Converter, Lib, and the elements of plist.

Two tests are failing--and they fail in the same way. Failing test: bad. Failing on a highly specific super-nested DATA_LIB_KEY dictionary: someone who knows the codebase can probably easily figure out the locus of the problem.

I have Rumsfeldian ignorance about many of the remaining issues: for me, these are unknown unknowns. The comments I wrote highlight the barriers caused by my ignorance.

bytes and PlistEncodable and _unstructure_data and especially # avoid encoding if converter supports bytes natively

I don't get it.

Probably, I don't get it because I don't really know what this module does. If the identifier PlistEncodable means "these types can be encoded to bytes," then that makes me think bytes shouldn't be part of the TypeAlias. And I think I would better understand _structure_data_inplace. Maybe PlistEncodable and PlistEncoded? IDK.

My point: I can't remember, or maybe I already made my point. idk.

I'm out of energy. I'll wait for some more guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants