adobe-type-tools / afdko

Adobe Font Development Kit for OpenType
https://adobe-type-tools.github.io/afdko/
Other
1.01k stars 166 forks source link

Add typing to otfautohint #1719

Open simoncozens opened 8 months ago

simoncozens commented 8 months ago

Description

otfautohint is a complex piece of code, and it's easy to get lost; it's even easier to make mistakes. Having typing information available makes it easier to catch such mistakes, and allows editors with Python type inferencing (Visual Code with pylance, Sublime Text with pyright, etc.) to provide type hints for variables and methods. This in turn helps to find typos, dead code, and so on.

Checklist:

skef commented 8 months ago

As you're probably realizing this has a lot of formatting related problems (flake8), some of which are due to edits for line length that you've removed.

josh-hadley commented 8 months ago

@simoncozens thanks for this contribution. It looks like there are a number of linter failures that are blocking the tests from running. I recommend that you run flake8 using the config from this repo, fix all reported issues, and resubmit. Once we get the linter + tests green we will have a closer review and see about getting it merged.

simoncozens commented 8 months ago

Yeah, sorry, I was assuming the code formatting was black/PEP8 so my editor runs black on save to keep things tidy. I'll try and put things back into your preferred style.

skef commented 8 months ago

I think I would prefer a submission with the substantive changes and then (perhaps) a different submission with the large amount of stylistic changes. Or perhaps just skip the latter? Best to either mesh one's contributions with the existing style or talk in advance about what you want to change and why.

Edit: Ninja-ed.

skef commented 8 months ago

@josh-hadley Merging this would presumably require additional pytype github checks, so that notations like # pytype: disable=attribute-error won't just rot.

simoncozens commented 8 months ago

To be honest, it doesn't currently work with pytype. I started working with pytype, and as I added annotations I found myself in situations where a method returned an instance of its own class, which needs the Self type, and pytype doesn't currently support that. So I switched to mypy. In a couple of cases mypy doesn't like the Self return value, but it does a better job than pytype (and continues running, whereas pytype just stops and refuses to go on).

simoncozens commented 8 months ago

Coverage tests are sad because apparently you can't use weakref.ReferenceType as a generic in Python 3.8.

skef commented 8 months ago

One reason we ported psautohint to python was the hope that it could be modified by the broader community, in comparison to the C code which was only ever worked on by a small group of Adobe-internal folks while squinting hard and crossing their fingers.

This PR is both an external contribution and an effort to make further contributions easier, so in that sense it's what we want and were hoping for.

Still, there is a basic concern about keeping these annotations from rotting. Some users will have environments that will make good use of the typing. Others will just be editing the files and running the tests and may have no idea what this stuff is or, if they do, how to change the annotations as the code changes.

Obviously some sort of verifier that can be run as part of the GitHub checks would be ideal, but it sounds like pytype can't do that and mypy might or might not be able to.

Do you have thoughts on this? Basically, how to maintain types in Python assuming a wide variety of users with different levels of experience and tooling?

Also: Keep in mind that we haven't been doing this internally, so it make take a while just to digest the implications of typed Python on our end.

anthrotype commented 8 months ago

FWIW annotations are pretty useless unless they are actually checked by a typechecker

anthrotype commented 8 months ago

ok, well.. self-documentation sure, but they then tend to rot as @skef noted.

simoncozens commented 8 months ago

Obviously some sort of verifier that can be run as part of the GitHub checks would be ideal, but it sounds like pytype can't do that and mypy might or might not be able to.

We can certainly get mypy set up in the CI, and add a disable comment to turn off the Self checks.