-
Notifications
You must be signed in to change notification settings - Fork 36
Added typing #70
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
Added typing #70
Conversation
…ing typing conflicts are probably bugs.
…mport. Added # pylint: disable=cyclic-import
…ECKING for pytest peace of mind.
This is a big one, so give us some time to look over this, but thanks for doing it! Heads up that it may be easier to do this review in a few rounds, tackling different things each time, so its more manageable. |
I expect it to take a few iterations, I'm expecting to have to make some changes. Please take your time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @BiffoBear !
I have a few relatively minor requests for it. I am going to try it out with hardware as well since this is one of the more sizeable libraries and typing additions. Will report back here after testing is carried out.
…Any' types for unused parameters. Replaced with types from CPython.
@FoamyGuy Thanks for your kind and thorough review. Requested changes made. I've also removed all the 'Any' types and replaced them with the types expected by the CPython equivalents. |
There was a problem hiding this 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. Tested successfully with simpletest example on Feather S2 TFT 8 beta4.
Thank you for taking this on @BiffoBear!
Updating https://github.com/adafruit/Adafruit_CircuitPython_MLX90640 to 1.2.14 from 1.2.13: > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#30 from tcfranks/main > Add .venv to .gitignore Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.16 from 1.12.15: > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#70 from BiffoBear/add_typing > Add .venv to .gitignore Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 1.0.1 from 1.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#27 from spovlot/response-header-overwrite > Add .venv to .gitignore Updating https://github.com/adafruit/Adafruit_CircuitPython_Logging to 5.1.0 from 5.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_Logging#44 from vladak/multiple_handlers > Add .venv to .gitignore Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
closes #45 Added typing to all modules. Made two small edits to improve typing compatibility.
In
adafruit_wiznet5k.adafruit_wiznet5k_ntp.__init__()
changedutc: int
toutc: float
as some time zones are on the half hour. Addedint()
toget_time()
to allow for a float argument being passed.In
adafruit_wiznet5k.adafruit_wiznet5k_dns.__init__()
changedself._host = 0
toself._host = b""
to remove a warning about it not having ato_int()
method.self._host
is set to abytes
object ingethostbyname()
and is read as a bytes object in_build_dns_question()
but nowhere else.I am confident that these changes don't break the code.
There are three other type conflicts that are more complex and have not been addressed.