Implement type checking with github actions

[roberto-arista]

Hey @benkiel and @knutnergaard,

I took a quick look into this and have a couple of questions:

• Should I continue working on the v1 branch?
• Should the checks be implemented using tox?
• @knutnergaard, did you use mypy or pyright when updating the code? I ran a quick scan of the v1 codebase with mypy error highlighting and noticed several issues.

Looking forward to your input! 👋

[knutnergaard]

@roberto-arista

I used mypy when updating the code and reported the issues which I was unable to solve in #739, but these are, at least for the most part, related to conflicts with the base.py module.

[roberto-arista]

If these can't be solved and we want to implement type checking in CI, we need to flag each of them with # type: ignore. Do you see other options?

[knutnergaard]

No, I agree, but I think these errors can be solved. I'm just not comfortable making adjustments to the base module logic without a second opinion from someone with more intimate knowledge of the codebase.

We could make a list of all unique errors in the annotated modules, and make a PR to solve them.

[benkiel]

@roberto-arista Yes, keep to the v1 branch, and yes have tox run the checking

[knutnergaard]

These mypy errors are currently unresolved:

1. [resolved in https://github.com/Resolve mypy errors. #807] Incompatibility in signature of _reprContents between BaseObject and it's subclasses, e.g.,

   glyph.py:85: error: Signature of "_reprContents" incompatible with supertype "BaseObject"  [override]
   glyph.py:85: note:      Superclass:
   glyph.py:85: note:          @classmethod
   glyph.py:85: note:          def _reprContents(cls) -> List[str]
   glyph.py:85: note:      Subclass:
   glyph.py:85: note:          def _reprContents(self) -> List[str]

2. [resolved in https://github.com/Resolve mypy errors. #807] Incompatibility in signature of isCompatible between InterpolationMixin and it's subclasses, e.g.,

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

3. [resolved in https://github.com/Resolve more mypy errors. #808] base.reference returning a wrapper rather than the actual object , e.g.,

   contour.py:125: error: Incompatible types in assignment (expression has type "Callable[[], Any]", variable has type "Optional[BaseGlyph]")  [assignment]

4. [resolved in https://github.com/Resolve mypy errors. #794] The annotated return type of isCompatible being str rather than BaseCopatibilityReporter in InterpolationMixin subclasses, e.g.,

   contour.py:534: error: "str" has no attribute "fatal"  [attr-defined]
   contour.py:534: error: "str" has no attribute "warning"  [attr-defined]
   contour.py:535: error: "str" has no attribute "fatal"  [attr-defined]
   contour.py:537: error: "str" has no attribute "warning"  [attr-defined]

   This annotation is based on original documentation, but probably should be the appropriate BaseCompatibilityReporter subclass.

5. [resolved in https://github.com/Add abstract members. #803] References to attributes not defined in class or base class, e.g.,

   contour.py:657: error: "BaseContour" has no attribute "_reverseContour"
   layer.py:52: error: "_BaseGlyphVendor" has no attribute "defaultLayer"  [attr-defined]
   

6. [resolved in https://github.com/Resolve more mypy errors. #808] Former weakrefs still being called in getters or setters of parent objects, e.g.,

   lib.py:112: error: "BaseFont" not callable  [operator]

7. [resolved in https://github.com/Add abstract members. #803] SelectionMixin. _getSelectedSubObjects passing _BaseGlyphVendor while expecting CollectionType[Any]:

   ayer.py:477: error: Argument 1 to "_getSelectedSubObjects" of "SelectionMixin" has incompatible type "_BaseGlyphVendor"; expected "Union[List[Any], Tuple[Any, ...]]"  [arg-type]
   layer.py:498: error: Argument 1 to "_setSelectedSubObjects" of "SelectionMixin" has incompatible type "_BaseGlyphVendor"; expected "Union[List[Any], Tuple[Any, ...]]"  [arg-type]
   

@roberto-arista @benkiel This is what I have so far. Any suggestions on how to fix these, preferably without the use of type: ignore?

[roberto-arista]

Hey @knutnergaard!

1. they return the same type, is it because of the superclass method being a classmethod?

To be honest, I don't know the code base well enough to provide a solution for any of these issues, sorry.

[knutnergaard]

@roberto-arista Yes, that's correct. Though I see no reason why it should be a classmethod in the BaseObject class.

[benkiel]

For 4: yes, should be BaseCompatibilityReporter

[knutnergaard]

Error 4 resolved in #794

[knutnergaard]

@benkiel Regarding error 1: Can the base method be changed to an instance method without breaking anything?

Regarding error 2: can the cls parameter be duplicated in the subclass method without breaking anything?

Those are the most straightforward ways to solve those issues.

[benkiel]

@knutnergaard I think the answer to 1 is yes. Need to poke more to understand 2

[knutnergaard]

@benkiel 5 can probably be solved by creating abstract members for what's missing. This would require all affected classes to inherit from abc.ABC and duplicating the members with abc.astractmethod. (See the docstring module for an example of this.)

Are you ok with adding this?

Edit 1: I should add that even thought this involves some duplication, it will make it much easier to keep track of which methods are created where, especially with sufficient documentation.

[knutnergaard]

@benkiel I've added abstract members to base.py in my abstract-members branch, which leaves the module completely in the green with regard to mypy errors.

[benkiel]

@knutnergaard I am ok with this, looking at the code. PR away and let me know what from the above then still needs answers.

[knutnergaard]

@knutnergaard I am ok with this, looking at the code. PR away and let me know what from the above then still needs answers.

@benkiel 5 solved in #803. Presuming it's ok to fix 1 based on your answer, this leaves 2, 3, 6 and 7.

[benkiel]

Yes, fine to fix 1 that way.
2: yes, I think it's fine to duplicate the cls
3: I think it should be returning a wrapper and not object, I am unsure
6: I am unsure here
7: Maybe this is because these are declared as @classmethods, maybe they shouldn't be?

[knutnergaard]

1 and 2 solved in #807.

[knutnergaard]

Seems 7 was indirectly solved in #803. That leaves 3 and 6.

[benkiel]

I think 3 and 6 are related.

[knutnergaard]

@benkiel I think they might be too. The way to go might be a simple Union type for the setters in question. I'll check it out.

[knutnergaard]

@benkiel @roberto-arista So, I solved the last two in #808. There is still a single error in annotations.py which I've overlooked. I'll look into that tomorrow.

[knutnergaard]

The annotations.py error was a quick fix, so all good.

[benkiel]

Pulled!

[roberto-arista]

👏

[knutnergaard]

@roberto-arista @benkiel #809 and #810 takes care of the errors thrown outside of base. This means that the whole code base should now pass, and integrating mypy into GitHub actions should be ok.