Skip to content

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

Merged
jh0ker merged 3 commits into
python-telegram-bot:masterfrom
lisitsky:fix-telegram-api-changes-raise-telegram-error-instead-of-native-404
Nov 9, 2016
Merged

Fix telegram API change, returning '404 Not found' #461
jh0ker merged 3 commits into
python-telegram-bot:masterfrom
lisitsky:fix-telegram-api-changes-raise-telegram-error-instead-of-native-404

Conversation

@lisitsky

@lisitsky lisitsky commented Nov 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@evieluvsrainbows evieluvsrainbows left a comment

Copy link
Copy Markdown

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

jh0ker commented Nov 8, 2016

Copy link
Copy Markdown
Member

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

@lisitsky

lisitsky commented Nov 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

jh0ker commented Nov 8, 2016

Copy link
Copy Markdown
Member

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

@lisitsky

lisitsky commented Nov 8, 2016

Copy link
Copy Markdown
Contributor Author

Ok. I've updated.

@jh0ker

jh0ker commented Nov 8, 2016

Copy link
Copy Markdown
Member

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