We tend not to have inline type hints (and I personally very much don't like them), but there are a few exceptions (e.g. _mathtext.py) so I guess it's up to you whether you want to leave them in.

Right – I saw that there are some files with type hints, and figured that the project might be in the process of adding them. I've found type hints pretty useful at work, but of course we should have a consistent style in the project. Has this been discussed in the past on the dev list or somewhere?

I'm probably among the most vocal opponents, but I'll defer to@QuLogic or@ksunden on this aspect.

I gave my opinion on our gitter, but I'll copy the main comment here for the sake of consolidation/potential for finding it in the future:

I would say type hints are a net positive in my opinion, though I acknowledge that there are problems (perhaps especially in a case like ours where APIs were designed well before type hints).

We went with stub files on initial implementation specifically to minimize some risk, specifically since the stub files have are not even loaded at runtime, there was no chance of them interfering.

However, I do think inline reduces a level ongoing maintenance risk, in particular the chance of the two files getting out of sync. (We have tooling/CI in place to help catch such things, but don't think it fully removes the risk)

The other factor is that stub files allow us draw the line at public APIs, and not have to worry about typing our internal logic. (Which has positives and negatives, positives being largely catching additional problems by type checker, negative largely being the surface area that needs to be covered.)

So in all, I think my personal recommendation would be a relatively slow uptake, where we keep the stub files for most public facing things in the interim, do inline type hints for internal logic when it makes sense to do so (e.g. when doing so is actively helpful for some refactor/feature addition/etc) then once the majority of internal logic has inline hints, consider inlining the hints of the more public facing APIs.

So looking at this example in particular, I think this does fall within my recommendation there. I am not personally opposed to it, but also not hard pulling for it. I do question a bit whether to advocate for "do minimal as you are working and motivated" or "the unit for adding type hints should be one file at a time". Doing the latter would carry the advantage that you get a better sense of how complete the hints are, but the disadvantage of asking people to type hint 3-4x what they were otherwise looking at (in this case, for example), which also impacts the review-ability of the PR as there are lots of changes that are actually orthogonal.

For what its worth, this particular file already has one function which got an inline typehint added in#27796

and@tacaswell responded

It is worth re-opening the discussion of in-line type hints
the world seems to have stabilized and in other projects I have had type hints catch actual bugs before I ran the code (but I have also had to spend 15 minutes trying to sort out how to placate the type checker for code that clearly works a couple of times)

The type hints were useful for me while working on this, since VS Code pointed out some obvious mistakes in real time. But I agree that it is not ideal to leave the file only partially annotated, since it does not pass a full type check in its current state.

I could remove the extra type hints from this PR and make a separate PR to add hints to the whole file?

Sure, let's just go for it. I'm not going to be able to resist this forever 🙄

I wonder if this may be clearer if you mutate buildchar_stack and postscript_stack in-place? this way you would write

glyphs=set();subrs=set()ifopcodein {...}:buildchar_stack[:]= []elifopcode=="seac":codes= ...;glyphs.update(...)buildchar_stack[:]= []elifopcode=="div":num2=buildchar_stack.pop()num1=buildchar_stack.pop()buildchar_stack.append(num1/num2)...returnglyphs,subrs

which feels perhaps more in the spirit of a postscript interpreter?

Could be! I'll think about this a little.

I moved this into a separate class with the stacks and glyph/subr sets as members. I agree that it looks clearer that way.

I think I disagree here... the English verb "set" is irregular in that way, but if you search for "subsetted" in the context of fonts, it seems to be fairly common, including in fonttools and various Adobe forums. See also the accepted answer tothis question and possibly the discussion offlied out in Steven Pinker'sWords and Rules.

Maybe just be a bit more verbose?

I don't disagree that this is the correct conjugation in the past tense, rather that these sentences are not in the past tense. It is stating what is and in the (foreseeable) future shall occur.

I reworded the whole paragraph to be hopefully more understandable on its own.

Should this (and _subset_subrs below) sort the glyphs (and subrs) to ensure reproducibility? (as set ordering changes over runs)

The subrs are already in order (the loop isfor i in range(n_subrs)) but sorting the glyphs is a good idea.

This reads well to me. Thanks!