-
Notifications
You must be signed in to change notification settings - Fork 6k
Add Tuple option to arguments type hints
#2167
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
Conversation
df45d2f to
e5a8cf4
Compare
|
Thanks for the new PR! I edited the PR description, so beth the issue and the old PR will be closed, when merging. |
|
Okay. I've just rebased the branch to current master and fixed the conflicts. |
Bibo-Joshi
left a comment
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 had a look at the changes and left 2 comments. Some more general notes:
- I think it's worth introducing a shortcut type for
Union[T, List[T], Tuple[T]]intelegram.utils.types. MaybeSTLfor single, tuple, list? that should shave some typing for future changes - Use of
Iterable: see my comment - I don't think that this PR should make any changes to non-signature code or tests. Basically the changes in this PR should not cause mypy to be unhappy. Adding tuple type hints where possible is merely a convenience extension of documentation and should not change the actual implementation.
|
About changing the implementation. When we started to take |
Single instance or list/tuple of instances. Issue: python-telegram-bot#2125
|
"STL" sounds too "Std Template Lib", so i chose "SLT". |
Frankly, compatibility with 3rd party libs shouldn't be the first priority when making changes. If they need a list, convert the input yourself, that's not PTBs job ;)
You have a point here. However, changing to everywhere in the code is a largely independent issue. As of now the aim of this PR is just to make type hinting a bit more generous. So please either only change signatures or go the full way and adjust the behaviour of every I'll try to have a closer look at the new changes soon. |
|
MyPy check fails on third-party lib calls inside PTB. If you still can merge such PR, so okay.
You know how to convince people (at least my kind). If you could ignore MyPy or loosen its checks, then say it, and i'll roll the implementation changes back. Even the generator optimization. |
Okay, apparently I misunderstood that. I had understood that mypy becomes invalid when using the changes with some 3rd party libs used in one of your projects. But in this case, I'll resort to "Basically the changes in this PR should not cause mypy to be unhappy. " |
|
Are there any change requests left? |
Bibo-Joshi
left a comment
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.
Are there any change requests left?
yes. aside from some minor comments, there is still use of Iterable and changes of non-signature code ;)
|
The only unnecessary change here was the generator optimization. I've removed it. |
Bibo-Joshi
left a comment
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.
Did you check if the :class:`telegram.utils.types.SLT`[:obj:`int`] thingies render correctly when building the docs?
|
About rendering thingies. Could not find a way to render them as hyperlinks and maintain readability of the remainder. |
|
@Poolitzer could you review this PR too? |
Poolitzer
left a comment
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.
LGTM
|
Hi. I'm glad the PRs are merged. But they still don't count for Hacktoberfest for some reason. Could you please add "hacktoberfest-accepted" label to both PRs? |
|
Thank you @Bibo-Joshi. But it didn't work. Hacktoberfest still doesn't count them. May be this is because i submitted these PRs too late (in the evening of 31th of October). Let's pretend this time i contibuted not for the tee)). |

closes #2125.
Based on #2152 with a little fix. closes #2152