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

Note: I removed a part ofBot.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.

Unify kwargs handling in Bot methods

95b4ee1

Bibo-Joshi added the bug 🐛 label

Apr 27, 2020

Bibo-Joshi requested a review fromPoolitzer

April 27, 2020 13:52

Poolitzer approved these changes

May 1, 2020

View reviewed changes

Copy link

Member

Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM.

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

Copy link

Contributor

n5y commentedMay 2, 2020•
edited
Loading

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

Having a separateapi_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.

Copy link

Contributor

n5y commentedMay 2, 2020•
edited
Loading

Theurl = '{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 singleBot._api_call(endpoint, data, timeout) or something like that.

Copy link

Contributor

n5y commentedMay 2, 2020•
edited
Loading

It seems like there's already bot api specific code inRequest.post (e.g.elif key == 'media':) so perhaps instead of spreading the logic across a whole new level ofBot._post /Bot._api_call it should be moved intoRequest.post. SinceRequest is already specialized for bot api it would make sense for it to havebase_url attribute. Or maybeapi_call can be made aRequest method and bot api specific logic moved into it out of bothRequest.post andBot._post.

The recent bug withreply_markup.to_json vsreply_markup.to_dict when uploading a file could be avoided ifRequest.post handled the conversion ofTelegramObject(or perhaps justReplyMarkup) like it already does forInputFile.

Copy link

MemberAuthor

Bibo-Joshi commentedMay 2, 2020•
edited
Loading

It seems like there's already bot api specific code inRequest.post (e.g.elif key == 'media':) so perhaps instead of spreading the logic across a whole new level ofBot._post /Bot._api_call it should be moved intoRequest.post. SinceRequest is already specialized for bot api it would make sense for it to havebase_url attribute. Or maybeapi_call can be made aRequest method and bot api specific logic moved into it out of bothRequest.post andBot._post.
The recent bug withreply_markup.to_json vsreply_markup.to_dict when uploading a file could be avoided ifRequest.post handled the conversion ofTelegramObject(or perhaps justReplyMarkup) like it already does forInputFile.

As I understood from the discussion with Noam about that recent bug,Requests only task should be the network stuff and mingling the "responsibilities" ofBot andRequest is not advisable. TheInputFile related stuff inRequest 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 inRequests not, for above reasoning.

Copy link

Contributor

n5y commentedMay 2, 2020•
edited
Loading

Where does network stuff end and bot stuff begin?
IfRequest can call.to_json() on value of Bot API specific "media" key it can probably do that for allTelegramObject values. It's about properly passing nested objects as multipart/form-data and there's already anif 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 requested changes

May 2, 2020

View reviewed changes

Copy link

Member

Eldinnie left a comment

Choose a reason for hiding this comment

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

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 addapi_kwargs to every bot method which will be added to the data_dict silently. and removing**kwargs seems good to me.

Bibo-Joshi added3 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 added2 commits

May 11, 2020 18:13

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

6bfb023

…hon-telegram-bot into fix-1918

Fix test_official

422209c

Bibo-Joshi requested review fromPoolitzer andEldinnie

May 11, 2020 17:34

Copy link

MemberAuthor

Bibo-Joshi commentedMay 13, 2020•
edited
Loading

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

Edit: Done

Bibo-Joshi added2 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

Copy link

Member

Poolitzer left a comment

Choose a reason for hiding this comment

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

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

Bibo-Joshi changed the base branch frommaster tov13

June 30, 2020 19:43

Merge branch 'v13' into fix-1918

cce34cc

Bibo-Joshi merged commit20ee6e1 intov13

Jun 30, 2020

Bibo-Joshi deleted the fix-1918 branch

June 30, 2020 20:07

Bibo-Joshi added a commit that referenced this pull request

Jul 10, 2020

Refactor handling of kwargs in Bot methods (#1924)

260b796

* 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

Bibo-Joshi added a commit that referenced this pull request

Jul 14, 2020

Refactor handling of kwargs in Bot methods (#1924)

6fd3d2f

* 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

Bibo-Joshi mentioned this pull request

Jul 18, 2020

[BUG] Behaviour of kwargs in bot methods#1918

Closed

Bibo-Joshi added a commit that referenced this pull request

Jul 19, 2020

Refactor handling of kwargs in Bot methods (#1924)

8e9938d

* 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

Bibo-Joshi added a commit that referenced this pull request

Jul 19, 2020

Refactor handling of default_quote (#1965)

8e94d0d

* 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

Bibo-Joshi added a commit that referenced this pull request

Aug 13, 2020

Refactor handling of kwargs in Bot methods (#1924)

124c93f

* 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

Bibo-Joshi added a commit that referenced this pull request

Aug 13, 2020

Refactor handling of default_quote (#1965)

5ccc1b6

* 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

Bibo-Joshi added a commit that referenced this pull request

Aug 16, 2020

Refactor handling of kwargs in Bot methods (#1924)

02b058c

* 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

Bibo-Joshi added a commit that referenced this pull request

Aug 16, 2020

Refactor handling of default_quote (#1965)

f74be43

* 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