Update documentation

[knutnergaard]

• add type annotations and edit/add documentation to base/font.py
• add new module for type annotation definitions: base/type.py
• add intersphinx mapping to python 3 inventory to documentation/source/conf.py

[benkiel]

Looks like fontshell.font may need to be updated here: tests are run via fontShell, fyi. Though, not sure tbh if that is the issue.

[knutnergaard]

I think the issue simply is that I changed the name of the new module from types.py to type.py, as per your request, right before pushing the commit to my local branch, but forgot to update the name in the import statement in font.py. Should I file a new PR or are you able to edit the file before merging?

[codecov]

Codecov Report

Attention: Patch coverage is 77.04918% with 28 lines in your changes missing coverage. Please review.

Project coverage is 78.13%. Comparing base (2ac6630) to head (0d97f96).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
Lib/fontParts/base/layer.py	75.75%	23 Missing and 1 partial ⚠️
Lib/fontParts/fontshell/layer.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #739      +/-   ##
==========================================
- Coverage   78.30%   78.13%   -0.17%     
==========================================
  Files          41       42       +1     
  Lines        6094     6157      +63     
  Branches     1017     1028      +11     
==========================================
+ Hits         4772     4811      +39     
- Misses       1116     1138      +22     
- Partials      206      208       +2     

Files with missing lines	Coverage Δ
Lib/fontParts/base/annotations.py	100.00% <100.00%> (ø)
Lib/fontParts/base/font.py	56.88% <ø> (+0.09%)	⬆️
Lib/fontParts/base/glyph.py	77.54% <ø> (+0.25%)	⬆️
Lib/fontParts/fontshell/layer.py	83.72% <0.00%> (-4.09%)	⬇️
Lib/fontParts/base/layer.py	60.95% <75.75%> (-4.40%)	⬇️

[knutnergaard]

The error was related to a misguided attempt of mine to fix a naming issue with the defaultLayer properties. I think the names of those are reversed, compared to their status as base vs. environment related.

Anyway, reversing the changes I made, fixed the problem, and for fear of breaking something more, I haven't looked further into this, but you should definitely take a look at those properties.

[benkiel]

Hey @knutnergaard, I am very sorry, I was typing far too quickly in my reply about the name for types.py: I do thing that types.py is better than type.py, the latter is going to be very confusing. Better to change it now before we merge.

[knutnergaard]

Hey @knutnergaard, I am very sorry, I was typing far too quickly in my reply about the name for types.py: I do thing that types.py is better than type.py, the latter is going to be very confusing. Better to change it now before we merge.

I had some trouble with the lasts few commits from the terminal, but hopefully everything is ok now. Sorry about that!

I'll update layer.py once @typesupply gets back to me about my latest questions. :)

[knutnergaard]

@benkiel, In /base/layer.py at line 53 (in the _BaseGlyphVendor._setLayerInGlyph method saying "Instance of '_BaseGlyphVendor' has no 'defaultLayer' member". This missing attribute seemed like an oversight to me, so I just thought I'd mention it.

[benkiel]

@knutnergaard I think that _BaseGlyphVendor._setLayerInGlyph bit is because _BaseGlyphVendor is subclassed by BaseFont which sets the logic for defaultLayer. Does that make sense?

[knutnergaard]

@knutnergaard I think that _BaseGlyphVendor._setLayerInGlyph bit is because _BaseGlyphVendor is subclassed by BaseFont which sets the logic for defaultLayer. Does that make sense?

Yes, that makes sense, but I see this error also occurs in other cases, like here, in the BaseFont class:

def swapLayerNames(self, layerName: str, otherLayerName: str) -> None:
    ...
    self._swapLayers(layerName, otherLayerName) # Instance of 'BaseFont' has no '_swapLayers' member

I could not find any logic for _swapLayers in any other FontParts module.

BTW, after some revisions not yet committed, all type annotation and logic pass when running mypy on font.py except for these errors:

font.py:1360: error: "BaseFont" has no attribute "_swapLayers"  [attr-defined]
font.py:2017: error: Signature of "isCompatible" incompatible with supertype "InterpolationMixin"  [override]
font.py:2017: note:      Superclass:
font.py:2017: note:          def isCompatible(self, other: Any, cls: Any) -> Tuple[bool, str]
font.py:2017: note:      Subclass:
font.py:2017: note:          def isCompatible(self, other: BaseFont) -> Tuple[bool, str]
font.py:2095: error: "str" has no attribute "fatal"  [attr-defined]
font.py:2095: error: "str" has no attribute "warning"  [attr-defined]
font.py:2096: error: "str" has no attribute "fatal"  [attr-defined]
font.py:2098: error: "str" has no attribute "warning"  [attr-defined]

I've edited the line numbers to reflect my latest commit of font.py

[benkiel]

(and thank you for understanding the speed of review!)

[roberto-arista]

Hey there! I'd be happy to contribute and help with this process. I use fontParts exclusively in a IDE environment and I'd love to have annotations to improve my developer experience. What could be the best way to proceed? A separate PR for each fontParts.base module missing?

[benkiel]

@roberto-arista Yes, PR for seperate things to the v1 branch

[benkiel]

@knutnergaard Ok — quick look at the documentation module — WOW!

I think the best place to put it in the documentation folder, in a new folder, either helpers or tools. Doesn't make sense to put it in the main fontParts folder structure

[knutnergaard]

@knutnergaard Ok — quick look at the documentation module — WOW!

I think the best place to put it in the documentation folder, in a new folder, either helpers or tools. Doesn't make sense to put it in the main fontParts folder structure

Great! I've put it in documentation > tools. I've done some minor refactoring, and improvements and added functionality to document normalization.

Also, I've added abstract versions of members used in the mixin but defined in the main Doctring class to avoid certain mypy errors. Now that annotation is introduced, this might be relevant for fontParts as well.

[knutnergaard]

@benkiel , after another pass at annotations in base.py and glyphs.py, only the following errors remain:

glyph.py:76: error: Signature of "_reprContents" incompatible with supertype "BaseObject"  [override]
glyph.py:76: note:      Superclass:
glyph.py:76: note:          @classmethod
glyph.py:76: note:          def _reprContents(cls) -> List[str]
glyph.py:76: note:      Subclass:
glyph.py:76: note:          def _reprContents(self) -> List[str]
glyph.py:2707: error: Signature of "isCompatible" incompatible with supertype "InterpolationMixin"  [override]
glyph.py:2707: note:      Superclass:
glyph.py:2707: note:          def isCompatible(self, other: Any, cls: Type[Any]) -> Tuple[bool, Any]
glyph.py:2707: note:      Subclass:
glyph.py:2707: note:          def isCompatible(self, other: BaseGlyph) -> Tuple[bool, str]

Is there any reason why these methods are defined as class methods in BaseObject?
I'll commit these changes once your initial review is done.

[knutnergaard]

@benkiel, regarding bPoint.py can you supply the type descriptions in the docstring table of BaseBPoint.type? I'm not sure how to describe them, and I feel like they should have a more thorough description than 'corner' or 'curve'.

[benkiel]

@knutnergaard Yes, will do. I had planned on reviewing this PR this morning/today, but...life. Know you are waiting on me, will get to it in short order.

[benkiel]

@knutnergaard for bPoint.py:

corner - a point where bcpIn and bcpOut are not smooth (linked)
curve - a point with bcpIn and bcpOut are smooth (linked)

[roberto-arista]

@knutnergaard I've seen that you started to work on bPoint.py in this PR. Should I close the other ones? Do you prefer work alone on this? Or, if it's helpful I can fork your repo and work on some other objects. Let me know.

[knutnergaard]

@knutnergaard I've seen that you started to work on bPoint.py in this PR. Should I close the other ones? Do you prefer work alone on this? Or, if it's helpful I can fork your repo and work on some other objects. Let me know.

@roberto-arista At present, all the commits to my own fork seem to update this PR automatically. I'm happy to use separate PRs like you've done, I'm just not sure how.

Anyway, my edits to bPoint.py is based on yours. I've basically just updated the documentation. I appreciate all the help you can give with the annotation, as I use those as basis when updating docstrings.

BTW, I'm not sure of the best workflow, but since annotations are a prerequisite for updating the docs, forking my repo might be the way to go. We might want to wait for @benkiel's review of this PR, though, which should be imminent. At least I'm not going to start working on a new module until then.

Thanks!

[roberto-arista]

Ok, let's wait for @benkiel's input on this PR. Since you are doing also work on the docs, I agree that it might make more sense to contribute to your fork first.

[knutnergaard]

Ok, let's wait for @benkiel's input on this PR. Since you are doing also work on the docs, I agree that it might make more sense to contribute to your fork first.

Great! In that case, to keep things organised, should I create separate branches for annotations and docs or should I branch each module, like you've done?