Skip to content

adding type annotations for adafruit_turtle.py #35

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.

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

Merged
merged 5 commits into from
Sep 4, 2023

Conversation

rrahkola
Copy link
Contributor

@rrahkola rrahkola commented Apr 27, 2023

Fixes issue #27 .
NOTE: in order to satisfy type annotations, some functions were refactored to maintain immutability of function parameters. The approach was to use xn and yn in place of x1 and y1.

@rrahkola rrahkola mentioned this pull request Apr 27, 2023
35 tasks
@tekktrik
Copy link
Member

@kattni @FoamyGuy this PR adds things to .gitignore, do we want to keep them and patch them to other libraries to futureproof or keep them out for now?

@tekktrik
Copy link
Member

Also @FoamyGuy - we allow import of __annotations__ since I believe that is now in the CircuitPython firmware, but I think it's just used to allow forward references without quotes. Thoughts? We mostly just use quotes elsewhere.

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Thanks for the submission! Here's a few things I saw other than what's above!

Comment on lines 756 to 757
p = self.pos()
x0, y0 = (p[0], p[1])
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, was there a specific reason these were broken up into do different steps? Not an issue, just wondering why the function call and unpacking happen separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure of the original reason it was separated out. Honestly I find the original a bit easier to follow, I'm inclined to revert back to that for now. we could always change it in the future if we do end up finding benefits to it.

"""
Return True if the Turtle is shown, False if it's hidden."""
if self._turtle_group:
return True
return False

# pylint:disable=too-many-statements, too-many-branches
def changeturtle(self, source=None, dimensions=(12, 12)):
def changeturtle(
self, source: Any = None, dimensions: Tuple[int, int] = (12, 12)
Copy link
Member

Choose a reason for hiding this comment

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

I think source could be a Union of displayio.TileGrid and str, wrapped with Optional.

@tekktrik tekktrik linked an issue Apr 27, 2023 that may be closed by this pull request
35 tasks
@FoamyGuy
Copy link
Contributor

@kattni @FoamyGuy this PR adds things to .gitignore, do we want to keep them and patch them to other libraries to futureproof or keep them out for now?

It does make sense to me to go ahead and accept the changes in gitngore. I think those may be things generated by mypy if run locally which we wouldn't want checked in I don't think.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I've caught this PR up and had a look over, it's looking good to me now.

The latest commits make the following changes:

  • merge from main and resolve conflicts
  • adopt all changes request in review
  • run pre-commit
  • remove a few additional usages of Any in favor of more specific types.

Thanks for working on this @rrahkola and to @tekktrik for the initial review.

Comment on lines 756 to 757
p = self.pos()
x0, y0 = (p[0], p[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure of the original reason it was separated out. Honestly I find the original a bit easier to follow, I'm inclined to revert back to that for now. we could always change it in the future if we do end up finding benefits to it.

@FoamyGuy FoamyGuy merged commit f632227 into adafruit:main Sep 4, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Type Annotations
3 participants