Refactor kwargs handling in Bot methods

[Bibo-Joshi]

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]

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.

1. 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.
2. A user that does actually want to pass additional arguments to Bot API is not forced to deal with the warning.

[n5y]

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]

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]

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]

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.

[Bibo-Joshi]

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

Edit: Done

[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.