-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
More instance methods #963
Conversation
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.
In general looks good. But please see my comments (and check the travis log for failures).
telegram/bot.py
Outdated
@@ -1,6 +1,7 @@ | |||
#!/usr/bin/env python | |||
# -*- coding: utf-8 -*- | |||
# pylint: disable=E0611,E0213,E1102,C0103,E1101,W0613,R0913,R0904 | |||
# flake8: noqa E501 |
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.
As discussed, please see if the style used in https://github.com/python-telegram-bot/python-telegram-bot/pull/766/files#diff-8f1e0eb01d5bc4ab22d82229e1920dfeR311 helps to avoid the pep8 violation ;-)
telegram/bot.py
Outdated
@@ -1305,16 +1306,18 @@ def get_user_profile_photos(self, user_id, offset=None, limit=100, timeout=None, | |||
return UserProfilePhotos.de_json(result, self) | |||
|
|||
@log | |||
def get_file(self, file_id, timeout=None, **kwargs): | |||
def get_file(self, file_or_file_id, timeout=None, **kwargs): |
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.
maybe just file
?
I don't like the long variable name.
In Updater.__init__
we've used two explicit variables (bot
& token
) to avoid a similar dilemma.
:class:`telegram.Message`: On success, instance representing the message posted. | ||
|
||
""" | ||
return self.bot.send_message(chat_id=self.id, *args, **kwargs) |
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.
bot
may be None
on some extreme cases. I believe that on these cases we should raise an explicit RuntimeError.
Though you may claim that we don't do that with Message.reply_*()
methods. So maybe we should fix there too?
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.
That was the exact point I was gonna make xD
Does it matter what error it throws?
It seems like a lot of code for some superficial error that's unlikely to ever happen (as far as I understand it, it will never happen if users don't do stupid stuff, right?)
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.
Explicit errors are much easier to debug, especially in async systems, rather than deciphering a stack trace.
If you feel that the code is good as it is I won't object. I simply suggest to reconsider.
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 will leave this as is.
telegram/files/audio.py
Outdated
""" | ||
Use this method to get basic info about the audio file and prepare it for downloading. | ||
For the moment, bots can download files of up to 20MB in size. The file can then be | ||
downloaded with :attr:`telegram.File.download`. It is guaranteed that the link will be |
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.
Is it really necessary to add ALL that information here? I think a link to Bot.get_file
should be more than sufficient.
(it will also make sure that documentation fixes will be done only in one place...)
Fails because of unrelated changes on telegram servers. |
Fixes #955
Adds more instance methods, namely:
This allows stuff such as:
As well as PhotoSize.get_file(), Video.get_file() etc.
Allows stuff like:
Additionally also does such that
bot.get_file(update.message.document)
works instead of
bot.get_file(update.message.document.file_id)