Skip to content

Improve InvalidToken Error Message #3167

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.

Sign up for GitHub

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

Jump to bottom
Merged
Bibo-Joshi merged 9 commits into from
Aug 3, 2022
Merged

Improve InvalidToken Error Message #3167

Bibo-Joshi merged 9 commits into from
Aug 3, 2022

Conversation

@harshil21
Copy link
Member

@harshil21 harshil21 commented Jul 23, 2022 •
edited
Loading

Deletes the Bot._validate_token step since bot tokens could change in the future. Now whenever an InvalidToken exception is raised, the token is printed along with it so the user can see where they went wrong.

Original proposal:

Improves PTB's verification of bot tokens and also adds a better error message along the way. Suppose your bot token is 123456:AAF5_-y.ert. So the error message now shows:

E telegram.error.InvalidToken: Illegal character found in bot token: 
E 123456:AAF5_-y.ert
E               ^

We could also add another check where we assert that the no. of chars after : is 34 or 35. I say 34 since in https://core.telegram.org/bots/#creating-a-new-bot it's 34, but I have only seen it be 35 though. I didn't add this since it might be too strict and could have false positives in the future?

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs and all suitable __all__ s

@harshil21 harshil21 added this to the v20.0a3 milestone Jul 23, 2022
Bibo-Joshi
Bibo-Joshi reviewed Jul 24, 2022
View reviewed changes
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.

I like the ideas! but I'd really like to avoid false positives here. Even the current checks make assertions that are not build on explicitly documented behavior:

  • token not containing spaces is not documented (but that's sure to make sense)
  • part before : being all digits is not documented (that's almost sure as well, as it's the bots user id)
  • part before : being >= 3 is not documented. I've never seen user ids shorter than 5 I think, but still …

checking for invalid characters in addition seems a bit risky to me, tbh … unless ofc I missed some documentation on that …

@harshil21
Copy link
Member Author

harshil21 commented Jul 24, 2022

  • token not containing spaces is not documented (but that's sure to make sense)

yeah, we were already checking that before, so it's safe by now.

  • part before : being >= 3 is not documented. I've never seen user ids shorter than 5 I think, but still …

yeah I'm not sure why we were checking for that. Since it's just the bot id I'd be okay for deleting this check altogether.

checking for invalid characters in addition seems a bit risky to me, tbh … unless ofc I missed some documentation on that …

Yeah, I'll open an issue in the bot api repo for this soon and see if I get a concrete answer.

@harshil21 harshil21 mentioned this pull request Jul 24, 2022
Closed
@harshil21
Copy link
Member Author

harshil21 commented Jul 24, 2022

Well so the reply received in the issue above is not what I expected.. but I have another idea:

We could suggest to the user what went wrong with their token after receiving an error from getMe. This should de-risk it enough and make it safe. The error shown would be along the lines:

E telegram.error.InvalidToken: Not Found
E
E PTB suggests the following issue with the token:
E Illegal character found in bot token: 
E 123456:AAF5_-y.ert
E               ^

What do you think?

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jul 24, 2022

TBH, the answer makes me rather question if it's worth to keep the _validate_token step at all …

Bibo-Joshi
Bibo-Joshi reviewed Jul 28, 2022
View reviewed changes
Bibo-Joshi
Bibo-Joshi reviewed Aug 2, 2022
View reviewed changes
@Bibo-Joshi Bibo-Joshi merged commit 143db5f into master Aug 3, 2022
@Bibo-Joshi Bibo-Joshi deleted the add-err-msg branch August 3, 2022 06:16
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi left review comments

Assignees

No one assigned

Labels

🔌 enhancement pr description: enhancement

Projects

None yet

Milestone

v20.0a3

Development

Successfully merging this pull request may close these issues.

3 participants

@harshil21 @Bibo-Joshi