-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Explicit --local
mode
#3154
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
Explicit --local
mode
#3154
Conversation
# Conflicts: # telegram/_bot.py # telegram/ext/_applicationbuilder.py # telegram/ext/_extbot.py # tests/test_applicationbuilder.py # tests/test_sticker.py
I finally got around to dig into this again and with a little distance to what I've coded so far, I think it's appropriate to point out the difficulties with this PR. Quick recap: The main idea was that introducing an argument Bot(local_mode=True).send_document(chat_id, document=path/to/file.pdf) and Bot(local_mode=False).send_document(chat_id, document=path/to/file.pdf) to work, i.e. it could lift the current restriction that paths (as string or Indeed, this works fine in the setting described above. The problem are the classes
So what? IMO we have three options:
So far I've only looked back at this thread for ~2h so I really haven't decided which version I like best. |
I think that this is supposed to be a small improvement, easing the developing burden for devs juggling between local and non local setup. Adding bot (and all this complicated stuff you mentioned) to the Input* class(es) just to make this work for everything is out of the scope imo. I would thus vote to not include Input* and give them a doc warning that it doesn't support local bot api instance But what about making a LocalBotInput* class instead? This means more work for the developers of course (import the correct one, and switch if the code should support both), but then we would have the same default behavior for all classes/method (always open path/non url strings). |
I would agree with poolitzer and vote for option 2.
It does sound like too much work/complexity for a small benefit/consistency, so I don't like it too much |
All right, option 2 it is, then :) Regarding |
Okay, I think this is ready for a first round of review, then :) |
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.
LGTM
# Conflicts: # telegram/_bot.py
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.
Yes, this looks good to me!
# Conflicts: # telegram/_bot.py
closes #3114
TODO after merge: Update wiki and transition guide.
Checklist for PRs
.. versionadded:: version
,.. versionchanged:: version
or.. deprecated:: version
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst
(optional)__all__
s