+
Skip to content

Break off typing part of #3826 #3952

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 2 commits into
base: main
Choose a base branch
from
Open

Break off typing part of #3826 #3952

wants to merge 2 commits into from
+670 −113

Conversation

madig
Copy link
Collaborator

@madig madig commented Oct 8, 2025 •
edited
Loading

See #3826 for context.

This tries to port only the type hints (plus some import sorting and asserts to please the type checkers) and nothing else.

I think it would be nice to overload __getitem__ and get so that e.g. font["cmap"] is correctly typed as ttLib.tables._c_m_a_p.table__c_m_a_p 👀

@jorenham In case you're still interested :)

Closes #3954.

@madig madig requested a review from anthrotype October 8, 2025 18:18
@madig madig marked this pull request as draft October 8, 2025 18:26
jorenham
jorenham reviewed Oct 8, 2025
View reviewed changes
Copy link

@jorenham jorenham left a comment

Choose a reason for hiding this comment

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

I had a quick look and left some comments on the typing vs runtime separation

jorenham
jorenham reviewed Oct 8, 2025
View reviewed changes
Lib/fontTools/ttLib/ttFont.py
from typing import overload
from typing_extensions import Self

from fontTools.ttLib.tables import (
Copy link

Choose a reason for hiding this comment

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

How about from fontTools.ttLib import tables?

Copy link
Collaborator Author

@madig madig Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Then with ... -> "tables._c_m_a_p.table__c_m_a_p": ... I get "_c_m_a_p" is not a known attribute of module "fontTools.ttLib.tables" Pylance. Unless you know a trick, I have explicitly import all of them :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One could maybe change MetaTools/buildTableList.py to place an explicit __all__ = [ ... ] into the generated tables module.

Copy link

Choose a reason for hiding this comment

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

Hmm yea annoying... I suppose an __all__ would be best, but that's probably out of scope

@jorenham
Copy link

jorenham commented Oct 8, 2025 •
edited
Loading

Would it help if I'd open a PR to this branch with my proposed changes?

dscorbett
dscorbett reviewed Oct 8, 2025
View reviewed changes
@madig
Copy link
Collaborator Author

madig commented Oct 9, 2025

Would it help if I'd open a PR to this branch with my proposed changes?

Feel free!

@dscorbett @anthrotype Maybe I'm confused about Tag and str. What should I do here? Do we expect the Tag class, which extends str? Should we allow both? Or just str?

@madig madig force-pushed the add-ttfont-type-hints branch from 8c189ae to 3fc2594 Compare October 9, 2025 10:44
@madig madig requested a review from behdad October 9, 2025 11:02
@madig madig marked this pull request as ready for review October 9, 2025 11:04
@madig madig force-pushed the add-ttfont-type-hints branch from 3fc2594 to 18e2836 Compare October 9, 2025 15:37
@madig @schriftgestalt
Break off typing part of #3826
515fa76 
Co-authored-by: schriftgestalt <georg.seifert@mac.com>
@madig madig force-pushed the add-ttfont-type-hints branch from 18e2836 to 515fa76 Compare October 9, 2025 15:52
@madig
Copy link
Collaborator Author

madig commented Oct 9, 2025

@jorenham I'm confused about the _NumberT thing; At

fonttools/Lib/fontTools/ttLib/ttFont.py

Line 1346 in 515fa76

location = self["avar"].renormalizeLocation(location, self)
I tried to add to renormalizeLocation the return type dict[str, float], but the type checker didn't like it:

Type "dict[str, float]*" is not assignable to declared type "Mapping[str, _NumberT@normalizeLocation]"
  "dict[str, float]*" is not assignable to "Mapping[str, _NumberT@normalizeLocation]"
    Type parameter "_VT_co@Mapping" is covariant, but "float" is not a subtype of "_NumberT@normalizeLocation"
      Type "float" is not assignable to type "_NumberT@normalizeLocation"

@madig
Copy link
Collaborator Author

madig commented Oct 10, 2025

I have changed all typings of Tag to just str | bytes. There are some functions that don't do tag = Tag(tag) when they probably should, like in

fonttools/Lib/fontTools/ttLib/ttFont.py

Line 854 in 515fa76

del self.tables[tag]
, but that can be fixed later.

jorenham
jorenham reviewed Oct 10, 2025
View reviewed changes
Copy link

@jorenham jorenham left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'll try to work on this this weekend, so consider the comments "notes to self".
But I really like what you've done so far.

Lib/fontTools/ttLib/ttFont.py
from typing import overload
from typing_extensions import Self

from fontTools.ttLib.tables import (
Copy link

Choose a reason for hiding this comment

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

Hmm yea annoying... I suppose an __all__ would be best, but that's probably out of scope

@madig madig mentioned this pull request Oct 10, 2025
Open
@madig madig force-pushed the add-ttfont-type-hints branch from cce862b to 83142cb Compare October 14, 2025 14:30
@madig
Copy link
Collaborator Author

madig commented Oct 14, 2025

@jorenham The number type issue remains, otherwise I tried to simplify a bit.

@jorenham
Copy link

I'll try to work on this this weekend, so consider the comments "notes to self". But I really like what you've done so far.

I wasn't able to find the time to work on this, sorry about that.

@jorenham The number type issue remains, otherwise I tried to simplify a bit.

I'll take a look at it soon

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

Reviewers

@dscorbett dscorbett dscorbett left review comments

@anthrotype anthrotype Awaiting requested review from anthrotype

@behdad behdad Awaiting requested review from behdad

+1 more reviewer

@jorenham jorenham jorenham left review comments

Reviewers whose approvals may not affect merge requirements

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

@madig @jorenham @dscorbett
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载