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[bot]]

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 :warning:
Lib/fontParts/fontshell/layer.py	0.00%	4 Missing :warning:

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/robotools/fontParts/pull/739?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools) | Coverage Δ | | |---|---|---| | [Lib/fontParts/base/annotations.py](https://app.codecov.io/gh/robotools/fontParts/pull/739?src=pr&el=tree&filepath=Lib%2FfontParts%2Fbase%2Fannotations.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools#diff-TGliL2ZvbnRQYXJ0cy9iYXNlL2Fubm90YXRpb25zLnB5) | `100.00% <100.00%> (ø)` | | | [Lib/fontParts/base/font.py](https://app.codecov.io/gh/robotools/fontParts/pull/739?src=pr&el=tree&filepath=Lib%2FfontParts%2Fbase%2Ffont.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools#diff-TGliL2ZvbnRQYXJ0cy9iYXNlL2ZvbnQucHk=) | `56.88% <ø> (+0.09%)` | :arrow_up: | | [Lib/fontParts/base/glyph.py](https://app.codecov.io/gh/robotools/fontParts/pull/739?src=pr&el=tree&filepath=Lib%2FfontParts%2Fbase%2Fglyph.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools#diff-TGliL2ZvbnRQYXJ0cy9iYXNlL2dseXBoLnB5) | `77.54% <ø> (+0.25%)` | :arrow_up: | | [Lib/fontParts/fontshell/layer.py](https://app.codecov.io/gh/robotools/fontParts/pull/739?src=pr&el=tree&filepath=Lib%2FfontParts%2Ffontshell%2Flayer.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools#diff-TGliL2ZvbnRQYXJ0cy9mb250c2hlbGwvbGF5ZXIucHk=) | `83.72% <0.00%> (-4.09%)` | :arrow_down: | | [Lib/fontParts/base/layer.py](https://app.codecov.io/gh/robotools/fontParts/pull/739?src=pr&el=tree&filepath=Lib%2FfontParts%2Fbase%2Flayer.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=robotools#diff-TGliL2ZvbnRQYXJ0cy9iYXNlL2xheWVyLnB5) | `60.95% <75.75%> (-4.40%)` | :arrow_down: |

[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]

_swapLayers is here: https://github.com/robotools/fontParts/blob/6446bb65573da81178c15e6a9dede1b68d7bf3a5/Lib/fontParts/base/font.py#L994

[knutnergaard]

@benkiel @typesupply I've built a module to simplify the documentation process, according to the current style. I don't know where it fits in the current folder structure, but if you want to add it to the repository, I can make it available.

[benkiel]

Hey @knutnergaard, I'll look at the documentation module — could you just share it as a gist right now?

Also, what do you think about making this PR against a new branch, v1. Better docs and typing would bring things to a spot where 1.0 feels right, and doing things as PRs against a branch would help others contribute to the typing effort, in addition to breaking up PRs into more easily reviewed chunks.

As for this PR — it will take me a bit to go through everything, I plan on having my review done by the 23rd. I know that's slow, but I have to do it between client projects and other work, so it's going to take a bit, FYI. I don't want you to think it's been forgotten (and I'll comment as I go)!

One thing that I see right now is the Sphinx build is throwing some errors, but it could be my version of Sphinx. I'll troubleshoot and come back with issues I can't solve.

Lastly — I would love to hear about how you're using fontParts in your work, the music font project seems really interesting!

A huge thank you for this work — it's really well done and I know it's a lot!

[knutnergaard]

Hey @knutnergaard, I'll look at the documentation module — could you just share it as a gist right now?

Sure! It's here.

Also, what do you think about making this PR against a new branch, v1. Better docs and typing would bring things to a spot where 1.0 feels right, and doing things as PRs against a branch would help others contribute to the typing effort, in addition to breaking up PRs into more easily reviewed chunks.

Sounds good.

As for this PR — it will take me a bit to go through everything, I plan on having my review done by the 23rd. I know that's slow, but I have to do it between client projects and other work, so it's going to take a bit, FYI. I don't want you to think it's been forgotten (and I'll comment as I go)!

Thanks for the update. I realise that you have a day job, so no worries. :)

One thing that I see right now is the Sphinx build is throwing some errors, but it could be my version of Sphinx. I'll troubleshoot and come back with issues I can't solve.

I know. Please do.

Lastly — I would love to hear about how you're using fontParts in your work, the music font project seems really interesting!

I'm not doing much design work myself nowadays, but the library I've built, SMufoLib, extends the functionality of FontParts to aid in building fonts following the SMuFL standard, which provides mapping and metadata standards for music fonts. Music fonts are very different from text fonts, in that the glyphs are much less uniform in nature. The work is also generally less involved with regard to spacing and kerning and more involved when it comes to balancing the size and weight of different symbols. Most importantly, SMufoLib utilises the ufo font lib and glyph lib objects to store and provide access to SMuFL-specific metadata. It does not implement FontParts in the intended way, but inherits directly from the fontshell objects. As such, it's completely console-based.

Actually, using FontParts and the UFO format for this was kind of a last resort, due to the short-comings of the current FontLab scripting API(s). As it turns out, the lib objects provided excellent means for data storage which I originally did not consider when thinking about this library strictly in FontLab terms.

A huge thank you for this work — it's really well done and I know it's a lot!

You're very welcome and thanks! :)

[typesupply]

I'm not doing much design work myself nowadays, but the library I've built, SMufoLib, extends the functionality of FontParts to aid in building fonts following the SMuFL standard, which provides mapping and metadata standards for music fonts.

Wow wow wow wow wow! This is so cool! 🤩

[typesupply]

Also...

It does not implement FontParts in the intended way

This may not be exactly what I was thinking with environment implementations, but, gosh, this is so, so, so, so, so much the spirit of my intentions. And, this use of the lib is exactly what we made it for. Seeing fontParts used like this makes my day!

[benkiel]

@knutnergaard thanks for the flexibility! I changed this PR to be against a v1 branch, so we can work there. I'll get on reviewing this PR and we can make separate PRs to this branch to add/do other things.

SMufoLIb is so cool! So glad that lib is doing exactly what we hoped they could be used for building interesting things for folks that wouldn't make sense in a general tool/spec!

[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.

Hey @roberto-arista I realise I'm not really well versed with GitHub branches and different pull requests. For some reason, all the edits done in the master branch of my own fork is just linked to this pull request, so I've just continued to push all my edits to robotools:master like before.

Since I see you've created a dedicated branch for each module in your own fork, and @benkiel has suggested we work within a v1 branch, I thought I'd at least create a similar 'v1' branch in my fork, and then bring my own master up to date with robotools:master. However, considering that this pull request now is set up to merge only 2 commits, rather than the original 37, that seems to have been not such a good idea, and I'm not really sure how to resolve the conflict. I think I have to read up on GitHub best practices. 🙈

I appreciate any help you can give with the annotation. My work on bPoint.py is based on your edits. I've just added documentation, basically. I'm more than willing to commit my edits to separate PRs, or even add them to the ones you've created. I'm just not sure how to. Sorry about that.

[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?

[knutnergaard]

Changed mind about nomalizer text in docs, keeping to private methods by changing tense

Just to be clear, should these references consistently be in the future tense in all modules?

[benkiel]

@knutnergaard yes, future tense when the value obtained by the private method will be normalized in the public method, past tense when the value given to the private method has been normalized by the public method before being handed to the private method.

Hopefully that is clear (I know, confusing)

[knutnergaard]

Apart from adding documentation https://github.com/robotools/fontParts/pull/739/commits/9e869d55150b5ed9c55a450837dc888e1106a365 also adds type annotation where missing and improves code generally.

[benkiel]

APPROVED! Thank you so much for all this work, I know it is a lot!