Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve contour.py #782

Merged
merged 18 commits into from
Nov 29, 2024
Merged

Improve contour.py #782

merged 18 commits into from
Nov 29, 2024

Conversation

knutnergaard
Copy link
Contributor

  • Add annotation.
  • Add/improve documentation.
  • Make minor code improvements.

knutnergaard and others added 8 commits November 20, 2024 14:08
@knutnergaard
Improve contour.py.
db41c34 
- Add type annotation.
- Add/improve documentation.
- changed `BaseSegments.segments` to return `tuple` instead of `list`.
@knutnergaard
Change test_segments_empty to accomodate change in `BaseContour._ge…
28991cc 
…t_segments`.
@knutnergaard
Add improve remaining documentation.
f0f9961
@knutnergaard @github-actions
Format fixes by ruff
592bf55
@knutnergaard
erge branch 'v1' into contour
39e60b1
@knutnergaard
Solve mypy errors.
ac2adee
@knutnergaard
Resolve mypy errors.
5800ca8 
Remaining errors to be collected in robotools#775:

```zsh
contour.py:58: error: Signature of "_reprContents" incompatible with supertype "BaseObject"  [override]
contour.py:58: note:      Superclass:
contour.py:58: note:          @classmethod
contour.py:58: note:          def _reprContents(cls) -> List[str]
contour.py:58: note:      Subclass:
contour.py:58: note:          def _reprContents(self) -> List[str]
contour.py:125: error: Incompatible types in assignment (expression has type "Callable[[], Any]", variable has type "Optional[BaseGlyph]")  [assignment]
contour.py:125: error: Argument 1 to "reference" has incompatible type "BaseGlyph"; expected "Callable[[], Any]"  [arg-type]
contour.py:277: error: "BaseContour" has no attribute "_getIdentifierforPoint"; maybe "_getIdentifierForPoint" or "getIdentifierForPoint"?  [attr-defined]
contour.py:477: error: Signature of "isCompatible" incompatible with supertype "InterpolationMixin"  [override]
contour.py:477: note:      Superclass:
contour.py:477: note:          def isCompatible(self, other: Any, cls: Type[Any]) -> Tuple[bool, Any]
contour.py:477: note:      Subclass:
contour.py:477: note:          def isCompatible(self, other: BaseContour) -> Tuple[bool, str]
contour.py:532: error: "str" has no attribute "fatal"  [attr-defined]
contour.py:532: error: "str" has no attribute "warning"  [attr-defined]
contour.py:533: error: "str" has no attribute "fatal"  [attr-defined]
contour.py:535: error: "str" has no attribute "warning"  [attr-defined]
contour.py:657: error: "BaseContour" has no attribute "_reverseContour"  [attr-defined]
```
@knutnergaard @github-actions
Format fixes by ruff
0f4f2ee
@knutnergaard
Copy link
Contributor Author

Remaining errors after 5800ca8 to be collected in #775:

contour.py:58: error: Signature of "_reprContents" incompatible with supertype "BaseObject"  [override]
contour.py:58: note:      Superclass:
contour.py:58: note:          https://github.com/classmethod
contour.py:58: note:          def _reprContents(cls) -> List[str]
contour.py:58: note:      Subclass:
contour.py:58: note:          def _reprContents(self) -> List[str]
contour.py:125: error: Incompatible types in assignment (expression has type "Callable[[], Any]", variable has type "Optional[BaseGlyph]")  [assignment]
contour.py:125: error: Argument 1 to "reference" has incompatible type "BaseGlyph"; expected "Callable[[], Any]"  [arg-type]
contour.py:277: error: "BaseContour" has no attribute "_getIdentifierforPoint"; maybe "_getIdentifierForPoint" or "getIdentifierForPoint"?  [attr-defined]
contour.py:477: error: Signature of "isCompatible" incompatible with supertype "InterpolationMixin"  [override]
contour.py:477: note:      Superclass:
contour.py:477: note:          def isCompatible(self, other: Any, cls: Type[Any]) -> Tuple[bool, Any]
contour.py:477: note:      Subclass:
contour.py:477: note:          def isCompatible(self, other: BaseContour) -> Tuple[bool, str]
contour.py:532: error: "str" has no attribute "fatal"  [attr-defined]
contour.py:532: error: "str" has no attribute "warning"  [attr-defined]
contour.py:533: error: "str" has no attribute "fatal"  [attr-defined]
contour.py:535: error: "str" has no attribute "warning"  [attr-defined]
contour.py:657: error: "BaseContour" has no attribute "_reverseContour"  [attr-defined]

@knutnergaard knutnergaard marked this pull request as ready for review November 22, 2024 10:29
@knutnergaard
Copy link
Contributor Author

@benkiel I had to delete the __doc__ I initially added for the bPoints property to avoid a tox error. What's the reason this cannot/should not be documented?

@benkiel
Copy link
Member

benkiel commented Nov 22, 2024

@knutnergaard looks like the __doc__ for bPoints was just missing a comma after `"bPoints", adding that should resolve the issue and allow for the documentation

@knutnergaard
Copy link
Contributor Author

@benkiel Ah! Still failing, but looks like that's due to the pathlib thing.

@benkiel
Copy link
Member

benkiel commented Nov 22, 2024

@knutnergaard yup, its the path tests, just fixed that.

benkiel
benkiel requested changes Nov 26, 2024
View reviewed changes
Copy link
Member

@benkiel benkiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of things and a question for @typesupply

Lib/fontParts/base/contour.py Outdated Show resolved Hide resolved
Lib/fontParts/base/contour.py Outdated Show resolved Hide resolved
Lib/fontParts/base/contour.py Outdated Show resolved Hide resolved
Lib/fontParts/base/contour.py Outdated Show resolved Hide resolved
Lib/fontParts/base/contour.py Outdated Show resolved Hide resolved
Lib/fontParts/base/contour.py Show resolved Hide resolved
@benkiel
Copy link
Member

benkiel commented Nov 26, 2024

@typesupply I will look through code again: there may be a couple of more places where it's a may that should be a must, though it does look like some things are explicitly optional (such as the option to keep curve when deleting a point)

@knutnergaard
Copy link
Contributor Author

I think return raiseNotImplementedError() is more or less a dead give-away.

@knutnergaard knutnergaard requested a review from benkiel November 27, 2024 11:30
@knutnergaard
Copy link
Contributor Author

@benkiel Looks like tests fail due to a naming discrepancy going on between generateIdentifierForPoint and it's private equivalent. Both should surely have the same casing, right?

@knutnergaard knutnergaard mentioned this pull request Nov 28, 2024
Merged
benkiel
benkiel approved these changes Nov 29, 2024
View reviewed changes
Lib/fontParts/base/contour.py Show resolved Hide resolved
@benkiel
Copy link
Member

benkiel commented Nov 29, 2024

@knutnergaard the old casing shows up in a couple of places that needed fixing. We will need to note this breaking change

@benkiel benkiel merged commit 0039ae4 into robotools:v1 Nov 29, 2024
10 checks passed
@benkiel
Copy link
Member

benkiel commented Nov 29, 2024

@knutnergaard thank you for this! Merged!

@knutnergaard knutnergaard mentioned this pull request Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@typesupply typesupply typesupply left review comments

@benkiel benkiel benkiel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@knutnergaard @benkiel @typesupply