-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add stubs for python-jenkins #12582
Add stubs for python-jenkins #12582
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thank you for your contribution!
Here's a first pass review:
I won't leave a comment on every single line for this, but you can be more precise when it comes to the return type. For example, you can use -> list
if a method always returns a list.
(there's an exception to this rule of thumb when it comes to complex return type unions)
You can read the section "Some further tips for good type hints" at https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#conventions for more information
Concretely, this means that for all the methods returning JSON data, you can replace Sequence
with list
and Mapping
with dict
in the return annotation.
Wow thanks a lot for the review. Let me come back to that after my vacation in next week. |
Co-authored-by: Avasam <[email protected]>
This comment has been minimized.
This comment has been minimized.
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.
You still need to update the return types from Sequence
and Mapping
to list
and dict
where we know that specific type is always returned.
I won't leave a comment on every single line for this, but you can be more precise when it comes to the return type. For example, you can use
-> list
if a method always returns a list.(there's an exception to this rule of thumb when it comes to complex return type unions)
You can read the section "Some further tips for good type hints" at https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#conventions for more information
Concretely, this means that for all the methods returning JSON data, you can replace
Sequence
withlist
andMapping
withdict
in the return annotation.
This comment has been minimized.
This comment has been minimized.
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.
Almost there! It's our (relatively new) policy to explain any leftover Any
. So that we know which python typing limitation was hit at the moment of writing the stubs, and isn't left to future readers to guess (especially since we still have older stubs full of Any
instead of Incomplete
). This can be done either through comments, or by using a clearly named alias when the same concept is repeated.
For instance, all json objects will have a str
key, so all your dict[Any, ...]
can safely become dict[str, ...]
.
If you simply don't know the actual type, or don't want to bother completing the stubs more than you already have, you can use Incomplete
. Having a bunch of Incomplete
is fine. It's clear, easy to search for, and anyone with a bit of spare time can contribute to fill them in.
All Any
that represent a union of different possible JSON value types, can be replaced by an alias named something like _JSONValue
. For instance: dict[Any, Any]
--> dict[str, _JSONValue
.
But only use this if the type is truly something like dict[str, str] | list[str] | str
. If it's just an object you don't know the structure of, use dict[str, Incomplete]
instead.
Outside of avoiding stranded Any
, this is otherwise quite satisfactory !
Co-authored-by: Avasam <[email protected]>
This comment has been minimized.
This comment has been minimized.
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.
Thank you! I'll leave some time for another maintainer to come by if they wanna add anything. Looks good to me !
Co-authored-by: Jelle Zijlstra <[email protected]>
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
No description provided.