Unify pen argument names #2919

jenskutilek · 2022-12-09T12:47:29Z

Some pens used baseGlyphName and transform as argument names, others used glyphName and transformation. This PR unifies the argument names and adds some type annotations.

anthrotype · 2022-12-09T12:53:20Z

Lib/fontTools/cu2qu/ufo.py

@@ -91,7 +92,11 @@ def closePath(self):
    def endPath(self):
        self._add_segment('end')

-    def addComponent(self, glyphName, transformation):
+    def addComponent(
+		self,


mixing tabs and spaces?
feels weird we add typing only to this method and not the rest. Maybe for now just rename the parameters if you don't have time to add type hints to all.

C&P error ...

I've started to add typing annotations to the pens. But I had to stop now, because it gets complicated and even requires some code rewriting to pass the type checks. But at least there is more typing than before now :)

Hm, where would it require code rewriting?

I needed to add some assertions, otherwise the type checker would complain about variables possibly being None, e.g.:

https://github.com/fonttools/fonttools/pull/2919/files#diff-713c63d7dd345e61c8c06754b31811b60345f57b1b2c70fdf7bb8aaf7227c92aR408-R420

jenskutilek · 2022-12-09T12:57:57Z

The tests fail ... didn’t see it locally as tox didn’t run because of lxml installation problems. I’m on it …

benkiel · 2022-12-12T03:21:22Z

Putting in a warning here, lots of things that are not fontTools pull in pens/subclass pens; please be sure that you aren't introducing breaking changes

justvanrossum

I don't have time to do a full review, but I notice typing_extensions is sneaking in here as a new dependency. I don't have a strong opinion about that right now, but there was some discussion, also in the context of dropping support for Python 3.7.

justvanrossum · 2022-12-12T07:45:57Z

Lib/fontTools/pens/basePen.py

+
+
+PenGlyphSet: TypeAlias = Optional[Dict[str, Any]]
+PenPoint: TypeAlias = Tuple[float, float]


I don't agree that a "pen point" is a an x, y tuple. It is a sequence with length two, containing two numbers.

I don't think this can be modeled in Python with anything other than tuple[float, float]. There's an open issue for mypy with a "workaround": python/mypy#8441. The most straightfoward way still is to just call it Sequence[float].

madig · 2022-12-12T10:11:53Z

Putting in a warning here, lots of things that are not fontTools pull in pens/subclass pens; please be sure that you aren't introducing breaking changes

Hm, I'm not sure how one can meaningfully "be sure" about it, other than to drop the PR? Maybe a GitHub search can help find some API users.

madig · 2022-12-12T10:28:54Z

Lib/fontTools/pens/basePen.py

 				bezierSegments.append((pt1, pt2, pt3))
 				pt1, pt2, pt3 = temp, None, None
 	bezierSegments.append((pt1, points[-2], points[-1]))
 	return bezierSegments


-def decomposeQuadraticSegment(points):
+def decomposeQuadraticSegment(points: List[Union[PenPoint, None]]) -> List[Tuple[PenPoint, PenPoint]]:


You can from __future__ import annotations at the top and then rewrite this as def decomposeQuadraticSegment(points: list[PenPoint | None]) -> list[tuple[PenPoint, PenPoint]]: :)

Thanks! I wasn't sure if Python 3.7 can deal with that notation.

The annotations import was added in 3.7. You still need to import the typings for type aliases though... until we have 3.10 minimum.

@anthrotype actually, I'm wondering about the points argument, Is None really possible? The docstring says the last point must be an on-curve point, implying that it cannot be None?

Yes the last point in the list (and only the last) can be None, which indicates a closed TrueType contour containing only off-curve points and no on-curves..

Yeah, but in this instance? It's only called from

fonttools/Lib/fontTools/pens/basePen.py

Line 348 in 1688400

for pt1, pt2 in decomposeQuadraticSegment(list(points)):

where the last point is taken care of?

Good point you are right the caller handles that case already, if this function expects all points to be non-None that we can type it without the Optional, maybe add an assertion too

madig · 2022-12-12T10:30:23Z

Lib/fontTools/pens/boundsPen.py

@@ -27,8 +27,8 @@ def __init__(self, glyphSet, ignoreSinglePoints=False):
 		self.init()

 	def init(self):
-	    self.bounds = None
-	    self._start = None
+		self.bounds = None


What happened to the indentation here?

madig · 2022-12-12T10:32:04Z

Random thought: to avoid mixing typing and formatting changes (black?), we could first make a pens formatting PR and add the commit to a rev-ignore file as described in https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view and set up CI to check formatting of pens from then on.

jenskutilek · 2022-12-12T11:08:31Z

@madig I agree that it is probably better to tackle this in separate steps. Should I prepare a new branch that only fixes the formatting for all pen code?

jenskutilek · 2022-12-12T11:15:40Z

Putting in a warning here, lots of things that are not fontTools pull in pens/subclass pens; please be sure that you aren't introducing breaking changes

Hm, I'm not sure how one can meaningfully "be sure" about it, other than to drop the PR? Maybe a GitHub search can help find some API users.

Should we add deprecation messages when somebody uses the "old" argument names? The only things that actually changed are baseGlyphName → glyphName and transform → transformation in some pens addComponent arguments.

anthrotype · 2022-12-12T11:17:49Z

avoid mixing typing and formatting changes

+1

Also we should refrain from adding typing_extension dependency until we drop support for python 3.7 (#2878) (hopefully sometime early next year once Robofont goes stable with updated python runtime)

anthrotype · 2022-12-12T11:18:52Z

deprecation messages

a client would get TypeError and it's going to be too late for issuing deprecation warning..

jenskutilek · 2022-12-12T14:25:25Z

I've made a formatting-only PR in #2923.

benkiel · 2022-12-14T15:42:52Z

Ok, looked through this a bit more; these are positional arguments, so I am less concerned about breaking things, even in subclassing. I would just ask why the change of names still: baseGlyphName is used in reference to components, where that language is used for a bunch of things —so this seems to be a bit of bikeshedding, as is arguing about not changing it, so I'll drop my objections and apologize for the extra noise.

jenskutilek added 4 commits December 9, 2022 13:41

Annotate types for all SegmentPen.addComponent methods

4cac72d

Unify argument names for PointPens

eb63e3b

Annotate addPoint method defs

cd40cc5

Add annotations in hashPointPen

8715b5b

jenskutilek requested review from anthrotype and madig December 9, 2022 12:47

jenskutilek mentioned this pull request Dec 9, 2022

AbstractPointPen and GLIFPointPen use different argument names #2911

Open

anthrotype reviewed Dec 9, 2022

View reviewed changes

jenskutilek added 18 commits December 9, 2022 14:35

Fix indentation

923b750

Fix method call

efc0a47

Fix indentation

9d582f8

Add annotations to basePen

d033a79

Indentation

7fb9bad

Add annotations to areaPen

8b6e6b6

Add annotations to teePen

9ab26a9

Add blank lines

4142276

Add annotations to pointPen

fe5ba52

Add annotations to filterPen

aa73997

Add annotations to cairoPen

9366b9b

Add annotations to cocaPen

1b8546c

Add annotations to cu2quPen

7bf1edd

filterPen: process missing identifiers

3968435

Add annotations to transformPen (incomplete)

dd83889

Undo transformPen

21f72cb

Add TypeAlias for PenGlyphSet

e87f818

Add missing arg

1688400

justvanrossum reviewed Dec 12, 2022

View reviewed changes

madig reviewed Dec 12, 2022

View reviewed changes

madig mentioned this pull request Dec 22, 2022

Add typing to pens #2936

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify pen argument names #2919

Unify pen argument names #2919

jenskutilek commented Dec 9, 2022

anthrotype Dec 9, 2022

jenskutilek Dec 9, 2022

jenskutilek Dec 9, 2022

madig Dec 10, 2022

jenskutilek Dec 12, 2022

jenskutilek commented Dec 9, 2022

benkiel commented Dec 12, 2022

justvanrossum left a comment

justvanrossum Dec 12, 2022

madig Dec 12, 2022 •

edited

madig commented Dec 12, 2022

madig Dec 12, 2022 •

edited

jenskutilek Dec 12, 2022

madig Dec 12, 2022

madig Dec 22, 2022

anthrotype Dec 22, 2022

madig Dec 22, 2022

anthrotype Dec 23, 2022

madig Dec 12, 2022

madig commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

anthrotype commented Dec 12, 2022

anthrotype commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

benkiel commented Dec 14, 2022



		PenGlyphSet: TypeAlias = Optional[Dict[str, Any]]
		PenPoint: TypeAlias = Tuple[float, float]

Unify pen argument names #2919

Are you sure you want to change the base?

Unify pen argument names #2919

Conversation

jenskutilek commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenskutilek commented Dec 9, 2022

benkiel commented Dec 12, 2022

justvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madig Dec 12, 2022 • edited

Choose a reason for hiding this comment

madig commented Dec 12, 2022

madig Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madig commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

anthrotype commented Dec 12, 2022

anthrotype commented Dec 12, 2022

jenskutilek commented Dec 12, 2022

benkiel commented Dec 14, 2022

madig Dec 12, 2022 •

edited

madig Dec 12, 2022 •

edited