Skip to content

Added type annotation hints #23

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
Merged
merged 8 commits into from
Feb 15, 2022
Merged

Added type annotation hints #23

merged 8 commits into from
Feb 15, 2022

Conversation

tammymakesthings
Copy link
Contributor

Fixes issue #20
Added type annotation hints.

@tekktrik tekktrik linked an issue Feb 13, 2022 that may be closed by this pull request
Missing Type Annotations #20
Closed
14 tasks
tekktrik
tekktrik requested changes Feb 13, 2022
View reviewed changes
Copy link
Member

@tekktrik tekktrik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey! Thanks for the submission! Add my review comments, most of them are repeats of each other. Let me know if anything is unclear or if I can explain anything!

@tekktrik
Copy link
Member

Oh one more thing! In __init__() for the class, I don't think address can be None so while it has a default argument, it does expect it to always be int so the typing should be int

tekktrik
tekktrik requested changes Feb 14, 2022
View reviewed changes
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.

Just one more tweak and it looks good to me! Curious to know how the const() thing will play out. Thanks so much @tammymakesthings!

@FoamyGuy
Copy link
Contributor

@tammymakesthings and @tekktrik

Okay, I have tested this out using the LSM6DS33 library. It does turn out that using mpy-cross did not work for type annotated variables like these with the previous version of mpy-cross.

The newer version (which we changed to starting with 7.0.0 I believe) does support it.

But we currently have code in the CI actions that still builds the mpy file for the prior version also which will end up failing if it runs:

Adafruit_CircuitPython_AS7341 on  issue20-add-type-annotations [?] via 🐍 v3.8.10 
❯ ./mpy-cross.static-amd64-linux-6.3.0 adafruit_as7341.py 
dexpresed ing back (most recent call  File "adafruit_as7341.py", line 283
SyntaxError:  eclass  intll

So we'll either need to not use the types for variables for now. Or remove the process that is building the older version of mpy file. I'll mention it on discord and we can get input from folks about what would be the best path forward at the moment.

@tekktrik
Copy link
Member

Thanks for testing! Sounds like a plan!

@tammymakesthings
Copy link
Contributor Author

So we'll either need to not use the types for variables for now. Or remove the process that is building the older
version of mpy file. I'll mention it on discord and we can get input from folks about what would be the best path
forward at the moment.

Sounds good. Once we know the path forward, I'll adjust accordingly so we can wrap this up. For whatever it's worth, I have a modest preference for the second option from a "minimize technical debt" perspective, but I'm not heavily invested one way or the other and I don't know what downstream ramifications there might be with that course of action. I'll wait to hear what we decide.

@FoamyGuy
Copy link
Contributor

Alright actually I was mistaken and my environment was out of date a little bit. It does not automatically build 6.3.x anymore, I just had an old version of the build-tools on my machine from before that got changed.

So I think this is actually okay, I have asked in discord to verify that we are okay with breaking compatibility for 6.3.x, as long as that is okay then these types can stay (and similar ones could get added to other repos)

@FoamyGuy
Copy link
Contributor

I got confirmation that we have already broken compatibility with 6.x in some other cases so we are okay to do that here as well.

I think we are good to go on this one after the tweak for the last change request that tekktrik made.

@tekktrik
Copy link
Member

Hey just wanted to say that while I checked the commit history and don't see anything, @tammymakesthings please just double check I didn't accidentally make any changes to your PR! I'm doing my own separate PR for the library and was worried I may have pushed changes. I don't see anything but would feel bad if I didn't mention it!

@tammymakesthings
Copy link
Contributor Author

please just double check I didn't accidentally make any changes to your PR!

Nope, looks like everything's good! Committing that last tweak momentarily. Thanks, @tekktrik and @FoamyGuy - this was a good learning opportunity for me.

tekktrik
tekktrik approved these changes Feb 15, 2022
View reviewed changes
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.

Looks good to me!

@tekktrik
Copy link
Member

Thank you both @tammymakesthings and @FoamyGuy!

@tekktrik tekktrik merged commit 5c80e13 into adafruit:main Feb 15, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 16, 2022
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@8ce3914)
eae2100 
Updating https://github.com/adafruit/Adafruit_CircuitPython_ADT7410 to 1.3.2 from 1.3.1:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_ADT7410#20 from tammymakesthings/issue18-type-annotations
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_ADXL34x to 1.11.12 from 1.11.11:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_ADXL34x#33 from tammymakesthings/issue30-add-type-annotations
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_AS726x to 2.0.9 from 2.0.8:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_AS726x#19 from tammymakesthings/issue17-add-type-annotations
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_AS7341 to 1.2.4 from 1.2.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_AS7341#24 from tekktrik/doc/add-CV-classes
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_AS7341#23 from tammymakesthings/issue20-add-type-annotations

Updating https://github.com/adafruit/Adafruit_CircuitPython_BME280 to 2.6.10 from 2.6.9:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_BME280#60 from tekktrik/doc/add-typing
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_FocalTouch to 1.3.0 from 1.2.10:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_FocalTouch#24 from kmatch98/master
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP9600 to 1.2.3 from 1.2.2:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP9600#18 from angerer/Issue-17_TypeAnnotations
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_PN532 to 2.3.7 from 2.3.6:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_PN532#53 from jerryneedell/jerryn_none
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7789 to 1.5.5 from 1.5.4:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7789#29 from mlashley/main
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_TLA202X to 1.0.5 from 1.0.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_TLA202x#4 from tekktrik/doc/add-usage-example
  > Merge pull request adafruit/Adafruit_CircuitPython_TLA202x#5 from tekktrik/doc/add-typing
  > Fixed readthedocs build
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_VL6180X to 1.4.0 from 1.3.1:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_VL6180X#23 from JonasSchatz/main
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 8.2.1 from 8.2.0:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#158 from tannewt/packet_buffer_test
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#156 from tekktrik/doc/consolidate-readme

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE_BroadcastNet to 0.12.1 from 0.12.0:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE_BroadcastNet#28 from tannewt/cp_native_bridge
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE_BroadcastNet#26 from tekktrik/doc/consolidate-readme

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE_Radio to 0.5.0 from 0.4.2:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE_Radio#21 from tekktrik/feature/use-ble-consts-2
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_BusDevice to 5.1.5 from 5.1.4:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_BusDevice#80 from dhalbert/prune-mock-imports

Updating https://github.com/adafruit/Adafruit_CircuitPython_Debouncer to 1.5.0 from 1.4.1:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_Debouncer#35 from d-a-v/multi
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.5.14 from 2.5.13:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#94 from tekktrik/doc/add-pause-example
  > Post-patch cleanup

Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoKey to 1.0.5 from 1.0.4:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_NeoKey#6 from mattgoff/typeAnnotations
  > Consolidate Documentation sections of README

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 1.10.7 from 1.10.6:
  > Fixed readthedocs build
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#100 from FoamyGuy/gzip_error
  > Consolidate Documentation sections of README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FoamyGuy FoamyGuy FoamyGuy left review comments

@tekktrik tekktrik tekktrik 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.

Missing Type Annotations
3 participants
@tammymakesthings @tekktrik @FoamyGuy