10000 Annotate ufoLib by knutnergaard · Pull Request #3875 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Annotate ufoLib #3875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Annotate ufoLib #3875

wants to merge 23 commits into from
+839 −508

Conversation

knutnergaard
Copy link

No description provided.

@knutnergaard
Copy link
Author
knutnergaard commented Jun 27, 2025 •
edited
Loading

@anthrotype @madig I'm running into a type mismatch between the glyphSet parameter in ufoLib.converters.convertUFO1OrUFO2KerningToUFO3Kerning, which appear to expect Iterable[str] (or something along those lines), given the default value of () and ufoLib.UFOReader.getGlyphSet, which returns a GlyphSet. The conflict results in an error in this call:

class UFOReader(_UFOBaseIO):
    def _upConvertKerning(self, validate: bool) -> None:
        if self._upConvertedKerningData:
            ...
        else:
            ...
            # convert kerning and groups
            kerning, groups, conversionMaps = convertUFO1OrUFO2KerningToUFO3Kerning(
                self._upConvertedKerningData["originalKerning"],
                deepcopy(self._upConvertedKerningData["originalGroups"]),
                self.getGlyphSet(),
            )

Should the last argument in the call to convertUFO1OrUFO2KerningToUFO3Kerning actually be self.getGlyphSet().keys()? If not, and glyphSet is supposed to accept a GlyphSet, should't the default value for glyphSet be None?

@jenskutilek
Copy link
Collaborator

convertUFO1OrUFO2KerningToUFO3Kerning only checks for the presence of glyph names in the glyph set, so I think the last argument should indeed be Iterable[str].

@knutnergaard
Copy link
Author

@jenskutilek I see, but this means that the last argument in the call should be self.getGlyphSet().keys() rather than the current self.getGlyphSet().

@justvanrossum
Copy link
Collaborator

The point of this argument is membership testing. 8000 This works for sets, dicts and lists (although porentially not efficiently for the latter), as well as anything that implements __contains__, such as GlyphSet. Therefore I think collections.abc.Set the the most appropriate annotation.

@knutnergaard
Copy link
Author
knutnergaard commented Jun 28, 2025 •
edited
Loading

collections.abc.Set

I Take it you mean typing.Set, to be able to specify items type (Set[str]), or am I missing something?

If using such a strict annotation, we'll need to cast the glyphSet argument to set in addition to ensuring it's an iterable of glyph names, i.e., set(self.getGlyphSet().keys()). Maybe this is a bit much?

Correction: Off course, dicts can be coerced to sets, so it's a bit simpler than the above, but I'm not sure that's what we should do.

@jenskutilek
Copy link
Collaborator

GlyphSet.keys() is a bit weird, it casts to list instead of just returning GlyphSet.contents.keys(). Maybe a remnant from Python 2?

fonttools/Lib/fontTools/ufoLib/glifLib.py

Lines 565 to 568 in 75af368

# dict-like support
def keys(self):
return list(self.contents.keys())

@knutnergaard
Copy link
Author

@jenskutilek As you suspect, dict.keys() in Python 2 returned a list, so this is a way to ensure consistency across versions. I see no reason not to update it now, though.

@justvanrossum
Copy link
Collaborator

I Take it you mean typing.Set, to be able to specify items type (Set[str])

No I do not. We don't want the concrete type set (which typing.Set is a deprecated alias for), we want to check "does this object support membership testing", aka, does it implement __contains__.

https://docs.python.org/3/library/typing.html#typing.Set says:

Note that to annotate arguments, it is preferred to use an abstract collection type such as collections.abc.Set rather than to use set or typing.Set.

@justvanrossum
Copy link
Collaborator
justvanrossum commented Jun 29, 2025 •
edited
Loading

Maybe collections.abc.Set is still too narrow. We really only want to check that the object supports __contains__.

@knutnergaard
Copy link
Author

I Take it you mean typing.Set, to be able to specify items type (Set[str])

No I do not. We don't want the concrete type set (which typing.Set is a deprecated alias for), we want to check "does this object support membership testing", aka, does it implement __contains__.

https://docs.python.org/3/library/typing.html#typing.Set says:

Note that to annotate arguments, it is preferred to use an abstract collection type such as collections.abc.Set rather than to use set or typing.Set.

Thanks, You're absolutely right! My brain was stuck on Python 3.8.
If __contains__ is the only requirement, a union type might be the way to go.

@knutnergaard
Copy link
Author

@justvanrossum There is always collections.abc.Container if we want to be totally generic.

@knutnergaard

This comment was marked as duplicate.

Sign in to view
@justvanrossum
Copy link
Collaborator

There is always collections.abc.Container if we want to be totally generic.

Ah yes, that’s the perfect one for this argument.

knutnergaard
knutnergaard commented Jun 29, 2025
View reviewed changes
Lib/fontTools/ufoLib/glifLib.py
@@ -597,7 +631,9 @@ def getUnicodes(self, glyphNames=None):
unicodes[glyphName] = _fetchUnicodes(text)
Copy link
Author

Choose a reason for hiding this comment

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

text is defined as bytes while _fetchUnicodes expects str. Not sure how to solve this.

knutnergaard
knutnergaard commented Jun 29, 2025
View reviewed changes
Lib/fontTools/ufoLib/glifLib.py
@@ -612,7 +648,9 @@ def getComponentReferences(self, glyphNames=None):
components[glyphName] = _fetchComponentBases(text)
Copy link
Author

Choose a reason for hiding this comment

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

See line 631.

@knutnergaard knutnergaard marked this pull request as ready for review June 30, 2025 17:21
@knutnergaard
Copy link
Author

Didn't bother with the deprecated plistLib module. Let me know if that should be annotated as well.

@knutnergaard
Copy link
Author

Still a few kinks to iron out. Reconverting to draft.

@knutnergaard knutnergaard marked this pull request as draft July 1, 2025 07:37
@knutnergaard knutnergaard marked this pull request as ready for review July 1, 2025 12:49
@knutnergaard
Copy link
Author

All Mypy checks and tests passing.

jenskutilek
jenskutilek reviewed Jul 1, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenskutilek jenskutilek jenskutilek left review comments

@justvanrossum justvanrossum justvanrossum left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@knutnergaard @jenskutilek @justvanrossum
0