Skip to content

Fix telegram API change, returning '404 Not found' #461

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
jh0ker merged 3 commits into from
Nov 9, 2016
Merged

Fix telegram API change, returning '404 Not found' #461

jh0ker merged 3 commits into from
Nov 9, 2016

Conversation

@lisitsky
Copy link
Contributor

@lisitsky lisitsky commented Nov 8, 2016 •
edited
Loading

For bots with incorrect credentials telegram returns

HTTP/1.1 404 Not Found
... headers skipped ...

{
    "description": "Not Found",
    "error_code": 404,
    "ok": false
}

Because of 404 response status http library raises naive NotFound exception, which is not meant to be caught by a request wrapper. As a result test is broken. This patch catches the exception and raises an internal telegram.TelegramError('Invalid server response') as it was conceived before in test testInvalidSrvResp.

P.S. I'm new to python-telegram-bot, so comments are welcome ;)

evieluvsrainbows
evieluvsrainbows approved these changes Nov 8, 2016
View reviewed changes
Copy link

@evieluvsrainbows evieluvsrainbows left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@jh0ker
Copy link
Member

jh0ker commented Nov 8, 2016

Sweet, thank you! Perhaps it would make sense to add an exception class to telegram/error.py?

@lisitsky
Copy link
Contributor Author

lisitsky commented Nov 8, 2016 •
edited
Loading

Thank you for feedback! :)
Yep, it look through the telegram/error.py and think InvalidToken could suite well here.
In what else situations can we get 404? If we raise this exception in all of them will we mislead a user? How do you think? Or it will be better to coin another one?

@jh0ker
Copy link
Member

jh0ker commented Nov 8, 2016

Oh yes, raising InvalidToken sounds good to me. If we encounter other situations where a 404 is returned, we can adjust as necessary.

@lisitsky
Copy link
Contributor Author

lisitsky commented Nov 8, 2016

Ok. I've updated.

@jh0ker
Copy link
Member

jh0ker commented Nov 8, 2016

Neat! LGTM
I think you aren't in our AUTHORS.rst yet. If you want, please add yourself there and leave a comment. Then I'll merge.

@jh0ker jh0ker merged commit a2fddbe into python-telegram-bot:master Nov 9, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@evieluvsrainbows evieluvsrainbows evieluvsrainbows approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lisitsky @jh0ker @evieluvsrainbows