Skip to content
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

Use Sequence instead of Union[List[…], Tuple[…]] #3412

Merged
merged 23 commits into from Jan 1, 2023
Merged

Conversation

clot27
Copy link
Member

@clot27 clot27 commented Dec 8, 2022

Working on #3350 with this

closes #3350

@clot27 clot27 marked this pull request as draft December 8, 2022 14:04
@Viicos
Copy link
Contributor

Viicos commented Dec 9, 2022

As mentioned in the related issue, using Sequence can be problematic with str (see python/typing#256, I still haven't figured how to handle this case properly, so you might have some ideas!)

@clot27
Copy link
Member Author

clot27 commented Dec 11, 2022

As mentioned in the related issue, using Sequence can be problematic with str (see python/typing#256, I still haven't figured how to handle this case properly, so you might have some ideas!)

as mentioned in the issue itself, using Sequence[something other than str] already excludes str :)

@clot27 clot27 changed the title Use Sequence instead of Union[List[…], Tuple[…]] Use Sequence instead of Union[List[…], Tuple[…]] Dec 11, 2022
@Viicos
Copy link
Contributor

Viicos commented Dec 11, 2022

As mentioned in the related issue, using Sequence can be problematic with str (see python/typing#256, I still haven't figured how to handle this case properly, so you might have some ideas!)

as mentioned in the issue itself, using Sequence[something other than str] already excludes str :)

Indeed but I see that there's a few places where Sequence[str] is used in this PR, so maybe this would require isinstance checks as stated in the issue comment you mentioned :)

This typing issue really is a problem in my opinion, as you can see in the typing issue mentioned this caused quite some trouble in production environments for some people :/

@Bibo-Joshi
Copy link
Member

Indeed but I see that there's a few places where Sequence[str] is used in this PR, so maybe this would require isinstance checks as stated in the issue comment you mentioned :)

The meaning #3350 (comment) is that we have to make sure not to pass strings as list of chars to TG, e.g. for message.reply_text(text="Hi"), we must tell TG that the text is "Hi", not ["H", "i"].

In the cases of the parameters allowed_updates, custom_emoji_ids and options (the params currently typed as Sequence[str], such a check will have the following effect: If a user passes allowed_updates="message", we will pass the value "message" to TG and TG will probably complain because it expects a JSON array and not a string. So while a type checker will not complain, there will be exceptions at runtime, so I doubt that this will lead to hard-to-debug edge cases.

This typing issue really is a problem in my opinion, as you can see in the typing issue mentioned this caused quite some trouble in production environments for some people :/

You are right that providing a precise type annotation for this doesn't seem possible atm. Personally I'd still be okay with using Sequence[str] because

  • it's a general limitation of Pynthon that we can do little about
  • passing a false value to such a parameter will likely be easy to detect (see arguments above)
  • so far only 3 parameters are affected …

@Viicos
Copy link
Contributor

Viicos commented Dec 11, 2022

Yes I agree Sequence[str] should still be used as this is a limitation of the language itself. The way I'm doing it currently is using isinstance as a runtime check, but even without this check as you said TG will probably complain so this isn't really important.

(Some people suggested the idea of using a Diff type annotation (e.g. Diff[Sequence[str], str]), imo Annotated could also be used in the future when it will be correctly supported).

@clot27 clot27 marked this pull request as ready for review December 28, 2022 05:43
telegram/_bot.py Outdated Show resolved Hide resolved
@lemontree210 lemontree210 mentioned this pull request Dec 28, 2022
lemontree210 added a commit that referenced this pull request Dec 28, 2022
it is supposed to be in #3412 only
telegram/_bot.py Outdated Show resolved Hide resolved
telegram/_bot.py Outdated Show resolved Hide resolved
telegram/_bot.py Outdated Show resolved Hide resolved
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! :) Thanks for the PR! Harshil in fact already covered two comments that I had in the pipeline xD

telegram/_bot.py Show resolved Hide resolved
telegram/request/_requestparameter.py Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit 456b81d into master Jan 1, 2023
@Bibo-Joshi Bibo-Joshi deleted the use-sequence branch January 1, 2023 13:24
@Bibo-Joshi
Copy link
Member

Thank you very much for your work on this nice PR @clot27 🙂

@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Sequence instead of Union[List[…], Tuple[…]]
5 participants