robotools
`point.py` annotations
Hey @benkiel,
As agreed in PR #739, I will create a separate PR for each module. I'm trying to follow the same patterns that @knutnergaard used in his PR. If I understand the work done so far correctly, I only need to annotate the module in fontParts.base, right?
@knutnergaard, could you review my proposal to ensure it follows the same steps as yours?
Since the work so far has focused on font.py and glyph.py, I plan to start with the smaller objects and work my way up.
cc: @typemytype
Since the work so far has focused on
font.pyandglyph.py, I plan to start with the smaller objects and work my way up.
I've also annotated and revised layer.py and annotated base.py.
BTW, it's a good idea to run mypy on the files before committing changes.
@knutnergaard I receive this error from Mypy. I see an identical method all across fontParts, so I assume the code is correct. Do you see it as well?
@roberto-arista, Yes I get this as well. Likely it's caused by the fact that _contour is supposed to hold a weak reference to a BaseContour object, and should be annotated as such using weakref.ReferenceType. The base.reference function, however, returns the object instance itself, rather than a weak reference of it, so I'm not sure how this is supposed to work.
Can you clarify, @typesupply or @benkiel?
Hey @knutnergaard we used to do weakref, that's why the comment is there, but we don't anymore for reasons here: https://github.com/robotools/fontParts/issues/71#issuecomment-362708807
@benkiel I see. What's the purpose then of the base.reference function and how would you prefer we annotate this?
If there is no chance of implementing the weakref in the future, I would suggest jut removing the method call in _getContour and other similar places. Otherwise, I guess the notation will have to be Optional[Union[BaseContour, ReferenceType[BaseContour]]], in which case a type alias is probably in order.
@knutnergaard that's a @typesupply or @typemytype question. I'm hesitant to change that up as it has been working for years, but it may actually not matter and should have been cleaned up as you suggest when we dropped weakref. Wait until either chime in.
What's the purpose then of the base.reference function and how would you prefer we annotate this?
Hmmmm. This is a really good question. I'm hesitant to change it because it may blow things up, but I'm 70% sure it won't. For now, let's do the following:
- Open an issue for me to review the necessity of the
referencefunction. Add a link to #71. - Delete the commented weakref code and add some documentation. Something like this:
This code returns a simple function that returns the given object. This is a backwards compatibility function that is under review. See (new issue number). We used to use weak references, but they proved problematic (see issue #71), so this function was put in place to make sure existing code continued to function. The need for it is questionable, so it may be deleted soon.
Does that work for everyone? I'd review it now but I don't trust my brain power to make such a big decision at the moment.
@roberto-arista @knutnergaard a lot of things got added in the #739 PR. Maybe we need to add mypy checking?
Here's an example of how I implemented it for the type annotations drawBot PR I worked on: https://github.com/roberto-arista/drawbot/blob/53924359d74418147392d2b04805b328139fc099/.github/workflows/test.yml#L43-L48
@benkiel, I incorporated this into #739, so I think this can be closed. Can you confirm, @roberto-arista?
@knutnergaard or @roberto-arista do you want to add the mypy tests into the codebase? I'm not sure what I should (if anything) be ignoring, etc.
I have some questions about implementing the type checking, I'll open a separate issue to discuss them.