-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adding type hints #3107
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
Adding type hints #3107
Conversation
@dherrada If I understand correctly, I think @tannewt agrees with replacing the various bytestring unions with |
@dhalbert Ah, I hadn't seen that. Sure thing |
Should I be doing ulab? |
Right now the .pyi files of ulab live in CircuitPython. @v923z and I have discussed the possibility of moving them, and we'd like for it to happen, but I don't know whether he's started any of that work. Can you leave it for a future PR until there's a firm answer? |
No, I haven't yet had time for that, but it is definitely high on my list. |
The difference is that TypeVar is exactly one type in a single definition
that uses it in more than one place (typechecker must pick one of the
options and use in all places in a single definition), whereas Union is any
of the types in any of the positions it is used (i.e. one definition can
have different types in different places). EG
AnyWritableBuf = TypeVar['AnyWritableBuf', bytearray, array.array, mmap,
memview] # AnyStr style.
WritableBuffer = Union[bytearray, array.array, mmap, memview]
fun send_recv_tv(buf: AnyWritableBuf) -> AnyWritableBuf:
...
return buf
fun send_recv_un(buf: WritableBuffer) -> WritableBuffer:
...
return buf
buf = bytearray(...)
result_tv: bytearray = send_recv_tv(buf)
result_un: Union[bytearray, array.array, mmap, memview] = send_recv_un(buf)
You can see that result_tv is correctly typed, i.e. the typechecker knows
that send_recv_tv returns something that is the same type as its argument.
This is because AnyWritableBuf is replaced in all positions, input and
output in this case, by the same type, in this case bytearray.
Whereas the typing for result_un just lists all the possible types, i.e.
information about the input type isn't used.
…-- Howard.
On Tue, 7 Jul 2020 at 08:16, Scott Shawcroft ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In shared-bindings/busio/I2C.c
<#3107 (comment)>
:
> @@ -228,7 +228,7 @@ STATIC mp_obj_t busio_i2c_readfrom_into(size_t n_args, const mp_obj_t *pos_args,
}
MP_DEFINE_CONST_FUN_OBJ_KW(busio_i2c_readfrom_into_obj, 3, busio_i2c_readfrom_into);
-//| def writeto(self, address: int, buffer: bytearray, *, start: int = 0, end: int = None, stop: bool = True) -> Any:
+//| def writeto(self, address: int, buffer: bytearray, *, start: int = 0, end: int = None, stop: bool = True) -> None:
@hlovatt <https://github.com/hlovatt> what is the advantage of TypeVar
over Union? We're all new to Python types.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACAIV2LIT4KESXXI6LLUOGDR2JEKJANCNFSM4OO4XXPQ>
.
|
@hlovatt Thanks for the explanation! That's super helpful. I had never thought about that case. |
@dherrada What is the status of this PR? Are you waiting on someone? |
If it is me, let me know! |
@v923z Ar far as ulab goes, I don't believe I'm blocked on that. |
Ok, I replied on those two. They got lost because this PR is huge. Please make many smaller PRs next time. Thanks! |
@tannewt if you open up all the hidden conversations, beyond the two screenshots, you'll see more that have to do with displayio types. There doesn't seem to be a way to link to conversations. |
@tannewt Will do. |
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.
Ok, I've added more comments. @dherrada please mention me on anything else that you need me to clarify. Thanks!
@tannewt Thanks. I just made all of those requested changes that you clarified and tagged you on a few more that I'm still unsure about. |
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.
Tags worked well! Thank you!
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.
This is great! Thank you!
@dherrada Can you take a look at the conflict here? I just merge some other improvements to extract_pyi and this needs an update. Thanks! |
@tannewt Sure. I do have a bunch of changes to extract_pyi that I did locally and haven't pushed yet, but they should be in a separate PR. |
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.
Ok, thanks! Looks good!
No description provided.