robotools / fontParts

The replacement for RoboFab
MIT License
135 stars 44 forks source link

Revision of internal value types #751

Open knutnergaard opened 3 weeks ago

knutnergaard commented 3 weeks ago

This is a two-part issue. First, I've spent som time today annotating the normalizers.py mdule, and come to the realisation that since there are so many internal types that differ in their input and output scopes, we should probably be more specific when annotating than what (at least) I thought initially necessary. This means that the annotations.py module will have to be revised. Here's my suggested basic revision:

from __future__ import annotations
from typing import Dict, List, Protocol, Tuple, TypeVar, Union

from fontTools.pens.basePen import AbstractPen
from fontTools.pens.pointPen import AbstractPointPen

# Generic
T = TypeVar('T')

Pair = Tuple[T, T]
Quartet = Tuple[T, T, T, T]
Sextet = Tuple[T, T, T, T, T, T]
CollectionType = Union[List[T], Tuple[T, ...]]
CollectionQuartetType = Union[List[T], Quartet[T]]

# Builtins
IntFloatType = Union[int, float]

# Pens
PenType = AbstractPen
PointPenType = AbstractPointPen

# Bounds
BoundsInputType = CollectionQuartetType[IntFloatType]
BoundsOutputType = Quartet[float]

# Character Mapping
CharacterMappingType = Dict[int, Tuple[str, ...]]

# Color
ColorInputType = CollectionQuartetType[IntFloatType]
ColorOutputType = Quartet[float]

# Coordinate
CoordinateInputType = Union[List[IntFloatType], Pair[IntFloatType]]
CoordinateOutputType = Pair[IntFloatType]

# Kerning
KerningKeyInputType = Union[List[str], Pair[str]]
KerningKeyOutputType = Pair[str]
KerningDictType = Dict[KerningKeyOutputType, KerningKeyOutputType]

# Component Mapping
ReverseComponentMappingType = Dict[str, Tuple[str, ...]]

# Transformation
TransformationMatrixInputType = Union[List[IntFloatType], Sextet[IntFloatType]]
TransformationMatrixOutputType = Sextet[float]
TransformationInputType = Union[int, float, List[IntFloatType], Pair[IntFloatType]]
TransformationOutputType = Pair[float]
FactorInputType = TransformationInputType
FactorOutputType = TransformationOutputType
ScaleInputType = TransformationInputType
ScaleOutputType = TransformationOutputType
SkewAngleInputType = TransformationInputType
SkewAngleOutputType = TransformationOutputType

# Interpolation
InterpolatableType = TypeVar('InterpolatableType', bound='Interpolatable')

class Interpolatable(Protocol):
    """Represent a protocol for interpolatable types."""

    def __add__(self, other: InterpolatableType) -> InterpolatableType:
        ...

    def __sub__(self, other: InterpolatableType) -> InterpolatableType:
        ...

    def __mul__(self, other: FactorInputType) -> InterpolatableType:
        ...

Ideally I would like to do away with all sequential assignments or variable synonyms, but TransformationInType/TransformationOutType are possibly too generic to be useful and clear when denoting both factor, scale and skew angle, even though the actual input and output types are the same for all. Technically, color and bounds values also share the same type in CollectionQuartetType, but again, I hesitate to use this directly in the code, as it feels too generic.

I need some feedback on this before I start revising the annotation in the committed modules.

The second part of this issue is which of these values to document in objectref.valuetypes.index.rst and reference in other parts of the documentation. The ability to references certain container types is really handy, especially considering the many elements needed to be spelled out without them (e.g., ':ref:`type-transformation`' vs. ':class:`tuple` or :class:`list` of six :class:`int` or :class:`float` values'). However, as of now, this part of the documentation is incomplete, which in some cases results in dead links to non-existent references.

Any thoughts?

knutnergaard commented 3 weeks ago

Just a small addendum: Generic containers could perhaps work if the naming is clear and neutral enough. One possible option might be:

Pair = Tuple[T, T]
Quadruple = Tuple[T, T, T, T]
Sextuple = Tuple[T, T, T, T, T, T]
CollectionType = Union[List[T], Tuple[T, ...]]
PairCollectionType = Union[List[T], Pair[T]]
QuadrupleCollectionType = Union[List[T], Quadruple[T]]
SextupleCollectionType = Union[List[T], Sextuple[T]]

These would then be used in place of all the explicitly font-related type names (like Coordinate, Angle, Color, etc.).

benkiel commented 3 weeks ago

@knutnergaard will get you a reply by week's end, start of week has been really busy.

knutnergaard commented 3 days ago

@benkiel Have you thought about this?

benkiel commented 3 days ago

Yes, agree that the containers can be well named but not named with type terminology so that they are more reusable/generic. Ok that they aren't exactly specific here.

As for documentation, yes we should add all of these to the doc so that we can reference them. That documentation should be clear that these are generic and can be used for a variety of things.

knutnergaard commented 2 days ago

@benkiel Thanks, I'll create a pull request with the suggested more generic aliases.

To be clear, given that the types in annotations.py are aliases and will appear as their underlying type in the generated documentation (i.e., PairCollectionType[str] will appear as Union[List[str], Tuple[str, str]] with links to the appropriate Python docs), internal documentation aren't strictly necessary (maybe with the exceptions of TypeVars like InterpolatableType).

Of course, The docs could still have a common value types section documenting font-specific or more generic container types, and reference these in the object docstrings instead of being explicit in every single case. Whatever the style, I think it's important that this is totally consistent and well documented.