-
Notifications
You must be signed in to change notification settings - Fork 46
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
lint: add almost all annotations to pass mypy --strict
#50
Conversation
One remains, for a context manager's __exit__: adafruit_tinylora.py:240: error: Function is missing a type annotation for one or more arguments [no-untyped-def] Not sure how this should be annotated, but it's not annotated in upstream cpython 3.12 either. Added type aliases bytearray2, bytearray4, etc to signal that the bytearrays passed must be 2 bytes, 4 bytes, etc. In adafruit_tinylora_encryption.py, there's a "state" matrix for the AES implementation there. It was mixing str and byte/int, which apparently works fine on circuitpython but fails on cpython. Change the state matrix to a bytearray, as was likely intended. Fixes adafruit#50.
6d4dd68
to
7f4e154
Compare
One remains, for a context manager's __exit__: adafruit_tinylora.py:240: error: Function is missing a type annotation for one or more arguments [no-untyped-def] Not sure how this should be annotated, but it's not annotated in upstream cpython 3.12 either. Added type aliases bytearray2, bytearray4, etc to signal that the bytearrays passed must be 2 bytes, 4 bytes, etc. In adafruit_tinylora_encryption.py, there's a "state" matrix for the AES implementation there. It was mixing str and byte/int, which apparently works fine on circuitpython but fails on cpython. Change the state matrix to a bytearray, as was likely intended. Fixes adafruit#50.
7f4e154
to
bb68abe
Compare
S_BOX: Tuple[ | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
bytes, | ||
] = ( |
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.
@FoamyGuy what are your thoughts on this, which is the true, correct definition, vs Tuple[bytes, ...]
which is more readable but would need a docstring to specify that it expects 16 bytes
objects?
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.
If cpython/mypy supported the syntax, this ideally would be something like
S_BOX: Tuple[bytearray16 * 16]
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.
I think I'm in favor of Tuple[bytes, ...]
and the docstring specifying 16 byte
s.
If they were listed out like this It would be be somewhat cumbersome to count them by hand for someone trying to use it anyhow so having it mentioned in the docstring would be appreciated I think. And I think in this instance it's worth it to save the space in the code and sacrifice being 100% accurate on the number of them
Added type aliases bytearray2, bytearray4, etc to signal that the bytearrays passed must be 2 bytes, 4 bytes, etc. In adafruit_tinylora_encryption.py, there's a "state" matrix for the AES implementation there. It was mixing str and byte/int, which apparently works fine on circuitpython but fails on cpython. Change the state matrix to a bytearray, as was likely intended. Fixes adafruit#50.
8c686e1
to
201752e
Compare
@tekktrik PR all ready from me, and CI passing now; let me know if anything else needed! |
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 @samatjain
bytearray4: TypeAlias = Annotated[bytearray, 4] | ||
bytearray16: TypeAlias = Annotated[bytearray, 16] | ||
|
||
registeraddress: TypeAlias = Union[const, int] |
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.
registeraddress might be one to consider adding to circuitpython_typing
we definitely have many libraries with arguments for register addresses. Many of them are typed int
I believe, but giving them a descripting name that is more specific might be an improvement. @tekktrik you think that one might be worth having over there?
examples/tinylora_simpletest.py
Outdated
@@ -75,7 +85,7 @@ | |||
lora = TinyLoRa(spi, cs, irq, rst, ttn_config) | |||
|
|||
while True: | |||
data = bytearray(b"\x43\x57\x54\x46") | |||
data = bytearray4(b"\x43\x57\x54\x46") |
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.
We'll need to test this out on a device. I'm thinking it might raise an exception for these statements where the bytearray number types like bytearra4
are used since those are initially defined inside of the try block that will fail on the microcontroller (as intended) due to not having typing.
There are a few places in the code that could have the same issue if it does work like that. In the cases where they are used as type annotations I think it should be okay because microcontroller will ignore it.
If that is the case we might have to revert them to normal bytearray
even if mypy strict throws problems about it.
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.
Switched all of these back to bytearray
with an appropriate annotation, e.g.:
data: bytearray4 = bytearray(b"\x43\x57\x54\x46")
for this one.
# Conflicts: # adafruit_tinylora/adafruit_tinylora_encryption.py
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.
I merged main and added an exclusion for a pylint error with naming.
The current version of this is looking good to me.
Sorry for the delay in getting back to this one, it fell through the cracks for me. Thanks for working on this @samatjain!
Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP9808 to 3.3.21 from 3.3.20: > Merge pull request adafruit/Adafruit_CircuitPython_MCP9808#37 from jposada202020/updating_example Updating https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO to 5.7.2 from 5.7.1: > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#98 from ForgottenProgramme/mahe-typehint Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 4.2.0 from 4.1.0: > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#63 from michalpokusa/external-routes-websockets-sse Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoKey to 1.1.0 from 1.0.12: > Merge pull request adafruit/Adafruit_CircuitPython_NeoKey#11 from EAGrahamJr/all-the-buttons Updating https://github.com/adafruit/Adafruit_CircuitPython_RGBLED to 1.1.17 from 1.1.16: > Merge pull request adafruit/Adafruit_CircuitPython_RGBLED#23 from BiffoBear/Add_typing Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.16 from 2.2.15: > Merge pull request adafruit/Adafruit_CircuitPython_TinyLoRa#50 from samatjain/type-annotations Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
One remains, for a context manager's exit:
Not sure how this should be annotated, but it's not annotated in upstream cpython 3.12 either.
Added type aliases bytearray2, bytearray4, etc to signal that the bytearrays passed must be 2 bytes, 4 bytes, etc.
In adafruit_tinylora_encryption.py, there's a "state" matrix for the AES implementation there. It was mixing str and byte/int, which apparently works fine on circuitpython but fails on cpython. Change the state matrix to a bytearray, as was likely intended.