Refactor kwargs handling in Bot methods #1924

Bibo-Joshi · 2020-04-27T13:52:06Z

Closes #1918

Passes kwargs specified as api_kwargs to API
Adds a warning about api_kwargs not being guaranteed to work. I put this on top of Bot rather than on every function as the latter seemed a waste of space to me.
reduces code duplication:
- base_url is added in Bot._post instead of every function
- data.update(api_kwargs) called in Bot._post instead of every function
minor doc fix for set_sticker_set_thumb on the fly

Note: I removed a part of Bot.set_webhook that was intended for backwards compatibility, as it used **kwargs. However, git blame tells me that it's 4 years old and from API 2.3.1 (?). So I thinks it's fair to say that it's not needed anymore.

Poolitzer

LGTM.

I dont like that we support this, but if the majority thinks this is good, I am okay with it

n5y · 2020-05-02T11:22:49Z

It's perfectly possible to pass parameters with GET methods, it's one of 4 supported ways to pass parameters https://core.telegram.org/bots/api#making-requests . Alternatively, there's no need for those methods to use GET instead of POST.

Having a separate api_kwargs or similar is better than having **kwargs with warning.

An IDE can warn the user about misspelled kwargs before the code is run. Testing that code runs without raising an exception is more straightforward than testing that code runs without logging warnings.
A user that does actually want to pass additional arguments to Bot API is not forced to deal with the warning.

n5y · 2020-05-02T16:24:50Z

The url = '{0}/{endpointName}'.format(self.base_url) logic can probably be moved into _post/_get methods. This would also make it easier to call not yet supported api methods. And since there's no need to have both _get and _post they could be replaced with a single Bot._api_call(endpoint, data, timeout) or something like that.

n5y · 2020-05-02T17:25:09Z

It seems like there's already bot api specific code in Request.post (e.g. elif key == 'media':) so perhaps instead of spreading the logic across a whole new level of Bot._post / Bot._api_call it should be moved into Request.post. Since Request is already specialized for bot api it would make sense for it to have base_url attribute. Or maybe api_call can be made a Request method and bot api specific logic moved into it out of both Request.post and Bot._post.

The recent bug with reply_markup.to_json vs reply_markup.to_dict when uploading a file could be avoided if Request.post handled the conversion of TelegramObject(or perhaps just ReplyMarkup) like it already does for InputFile.

Bibo-Joshi · 2020-05-02T20:08:25Z

It seems like there's already bot api specific code in Request.post (e.g. elif key == 'media':) so perhaps instead of spreading the logic across a whole new level of Bot._post / Bot._api_call it should be moved into Request.post. Since Request is already specialized for bot api it would make sense for it to have base_url attribute. Or maybe api_call can be made a Request method and bot api specific logic moved into it out of both Request.post and Bot._post.

The recent bug with reply_markup.to_json vs reply_markup.to_dict when uploading a file could be avoided if Request.post handled the conversion of TelegramObject(or perhaps just ReplyMarkup) like it already does for InputFile.

As I understood from the discussion with Noam about that recent bug, Requests only task should be the network stuff and mingling the "responsibilities" of Bot and Request is not advisable. The InputFile related stuff in Request is just there to handle uploading files, which is network stuff.

Unifying the construction of the URL, seems a good idea to me, too. Putting it in Requests not, for above reasoning.

n5y · 2020-05-02T22:10:14Z

Where does network stuff end and bot stuff begin?
If Request can call .to_json() on value of Bot API specific "media" key it can probably do that for all TelegramObject values. It's about properly passing nested objects as multipart/form-data and there's already an if branch about properly passing ints and floats as multipart/form-data.

In context of dealing with Bot API, setting base url is about as close to network stuff as setting a proxy is. Request is not some abstract network wrapper at this point, it's made specifically for requests to Bot API, trying to generalize it might needlessly increase code complexity.

Eldinnie

I;m starting to lean towards @n5y 's argument. Not specifically we need to protect users from early mistakes, but that additional kwarg adding should be explicit. That way we can reduce the warnings.

The option to add api_kwargs to every bot method which will be added to the data_dict silently. and removing **kwargs seems good to me.

…o head of Bot class

…hon-telegram-bot into fix-1918

Bibo-Joshi · 2020-05-13T18:21:38Z

Just noticed that the doc strings of the file types get_file should also be updated. Will do soonish.

Edit: Done

# Conflicts: # telegram/bot.py # telegram/utils/request.py

Poolitzer

Great change! I love this solution and I thank you for putting this much effort into it. It looks way better now, great stuff.

* Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods

* Refactor handling of `default_quote` * Make it a breaking change * Pickle a bots defaults * Temporarily enable tests for the v13 branch * Temporarily enable tests for the v13 branch * Refactor handling of kwargs in Bot methods (#1924) * Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods * Refactor JobQueue (#1981) * First go on refactoring JobQueue * Temporarily enable tests for the v13 branch * Work on tests * Temporarily enable tests for the v13 branch * Increase coverage * Remove JobQueue.tick() # Was intended for interal use anyways # Fixes tests * Address review * Temporarily enable tests for the v13 branch * Address review * Dispatch errors * Fix handling of job_kwargs * Remove possibility to pass a Bot to JobQueue * Refactor persistence of Bot instances (#1994) * Refactor persistence of bots * User BP.set_bot in Dispatcher * Temporarily enable tests for the v13 branch * Add documentation * Add warning to Updater for passing both defaults and bot * Address review * Fix test

* Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods

* Refactor handling of `default_quote` * Make it a breaking change * Pickle a bots defaults * Temporarily enable tests for the v13 branch * Temporarily enable tests for the v13 branch * Refactor handling of kwargs in Bot methods (#1924) * Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods * Refactor JobQueue (#1981) * First go on refactoring JobQueue * Temporarily enable tests for the v13 branch * Work on tests * Temporarily enable tests for the v13 branch * Increase coverage * Remove JobQueue.tick() # Was intended for interal use anyways # Fixes tests * Address review * Temporarily enable tests for the v13 branch * Address review * Dispatch errors * Fix handling of job_kwargs * Remove possibility to pass a Bot to JobQueue * Refactor persistence of Bot instances (#1994) * Refactor persistence of bots * User BP.set_bot in Dispatcher * Temporarily enable tests for the v13 branch * Add documentation * Add warning to Updater for passing both defaults and bot * Address review * Fix test

* Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods

* Refactor handling of `default_quote` * Make it a breaking change * Pickle a bots defaults * Temporarily enable tests for the v13 branch * Temporarily enable tests for the v13 branch * Refactor handling of kwargs in Bot methods (#1924) * Unify kwargs handling in Bot methods * Remove Request.get, make api_kwargs an explicit argument, move note to head of Bot class * Fix test_official * Update get_file methods * Refactor JobQueue (#1981) * First go on refactoring JobQueue * Temporarily enable tests for the v13 branch * Work on tests * Temporarily enable tests for the v13 branch * Increase coverage * Remove JobQueue.tick() # Was intended for interal use anyways # Fixes tests * Address review * Temporarily enable tests for the v13 branch * Address review * Dispatch errors * Fix handling of job_kwargs * Remove possibility to pass a Bot to JobQueue * Refactor persistence of Bot instances (#1994) * Refactor persistence of bots * User BP.set_bot in Dispatcher * Temporarily enable tests for the v13 branch * Add documentation * Add warning to Updater for passing both defaults and bot * Address review * Fix test

Unify kwargs handling in Bot methods

95b4ee1

Bibo-Joshi added the bug 🐛 label Apr 27, 2020

Bibo-Joshi requested a review from Poolitzer April 27, 2020 13:52

Poolitzer approved these changes May 1, 2020

View reviewed changes

Eldinnie requested changes May 2, 2020

View reviewed changes

Bibo-Joshi added 3 commits May 10, 2020 21:40

Merge branch 'master' into fix-1918

53bb1cd

Merge branch 'master' into fix-1918

09842a1

Remove Request.get, make api_kwargs an explicit argument, move note t…

6e1539d

…o head of Bot class

Bibo-Joshi changed the title ~~Unify kwargs handling in Bot methods~~ Refactor kwargs handling in Bot methods May 11, 2020

Bibo-Joshi added 2 commits May 11, 2020 18:13

Merge branch 'fix-1918' of https://github.com/python-telegram-bot/pyt…

6bfb023

…hon-telegram-bot into fix-1918

Fix test_official

422209c

Bibo-Joshi requested review from Poolitzer and Eldinnie May 11, 2020 17:34

Bibo-Joshi added 2 commits May 16, 2020 15:13

Update get_file methods

d12f761

Merge branch 'master' into fix-1918

57750e1

# Conflicts: # telegram/bot.py # telegram/utils/request.py

Poolitzer approved these changes Jun 23, 2020

View reviewed changes

Bibo-Joshi changed the base branch from master to v13 June 30, 2020 19:43

Merge branch 'v13' into fix-1918

cce34cc

Bibo-Joshi merged commit 20ee6e1 into v13 Jun 30, 2020

Bibo-Joshi deleted the fix-1918 branch June 30, 2020 20:07

Bibo-Joshi mentioned this pull request Jul 18, 2020

[BUG] Behaviour of kwargs in bot methods #1918

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020

Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor kwargs handling in Bot methods #1924

Refactor kwargs handling in Bot methods #1924

Bibo-Joshi commented Apr 27, 2020 •

edited

Loading

Poolitzer left a comment

n5y commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

Bibo-Joshi commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

Eldinnie left a comment

Bibo-Joshi commented May 13, 2020 •

edited

Loading

Poolitzer left a comment

Refactor kwargs handling in Bot methods #1924

Refactor kwargs handling in Bot methods #1924

Conversation

Bibo-Joshi commented Apr 27, 2020 • edited Loading

Poolitzer left a comment

Choose a reason for hiding this comment

n5y commented May 2, 2020 • edited Loading

n5y commented May 2, 2020 • edited Loading

n5y commented May 2, 2020 • edited Loading

Bibo-Joshi commented May 2, 2020 • edited Loading

n5y commented May 2, 2020 • edited Loading

Eldinnie left a comment

Choose a reason for hiding this comment

Bibo-Joshi commented May 13, 2020 • edited Loading

Poolitzer left a comment

Choose a reason for hiding this comment

Bibo-Joshi commented Apr 27, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

Bibo-Joshi commented May 2, 2020 •

edited

Loading

n5y commented May 2, 2020 •

edited

Loading

Bibo-Joshi commented May 13, 2020 •

edited

Loading