Skip to content

[Discussion] kwargs for input-only classes (like InputMedia*) #2607

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

Closed
Bibo-Joshi opened this issue Jul 26, 2021 · 10 comments
Closed

[Discussion] kwargs for input-only classes (like InputMedia*) #2607

Bibo-Joshi opened this issue Jul 26, 2021 · 10 comments
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Milestone

Comments

@Bibo-Joshi
Copy link
Member

There are some TG classes that are used for input only, but where we still accept kwargs - which is mainly intended for the classes that we de-json TG data. This sometimes leads to confusion like here https://t.me/pythontelegrambotgroup/503803 b/c additional arguments are silently ignored.

This is somehow the same problem as in #1924.

I see three possible ways to proceed with this:

  1. keep **kwargs and do nothing
  2. Drop **kwargs for classes that are exclusively used for input. This has a minor risk of TG actually sending them back to us at some point …
  3. keep **kwargs but issue warnings when receiving unexpected arguments

I think we should at least try to apply any of the solutions consistently - not sure if that's currently the case.

@Bibo-Joshi Bibo-Joshi added 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Jul 26, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Jul 26, 2021
@Poolitzer
Copy link
Member

TG classes that are used for input only

If I understand this correctly, you mean these classes aren't used by Telegram and only by Users to make a request. So I dont get what you mean with "minor risk of TG sending them back to us"

@Bibo-Joshi
Copy link
Member Author

So I dont get what you mean with "minor risk of TG sending them back to us"

The important part here is "at some point", meaning after a future bot api release. E.g. TG might decide to to have get_my_command return a BotCommandScope for something. If the same release adds another attribute to BotCommandScope, the code would blow up, if BotCommandScope doesn't accept kwargs. Surely unlikely that something like this happens, but not impossible.

@Poolitzer
Copy link
Member

Ah I see. Thats a risk I would say is worth to take. I would drop kwargs and include the api kwargs parameter like we do with requests.

@Bibo-Joshi
Copy link
Member Author

Ah I see. Thats a risk I would say is worth to take. I would drop kwargs and include the api kwargs parameter like we do with requests.

My comment on this in dev chat seems to have gone unnoticed: What would api_kwargs be good for here? They wouldn't help us with de_json

@Poolitzer
Copy link
Member

... right, weird comment.

My stance is the same, I would take the risk that telegram might screw it up and rather have a better user experience and not swallowing errors.

@Bibo-Joshi
Copy link
Member Author

Breakfast idea: instead of blindly doing cls(**json_dict) in de_json, we can use inspect.signature(cls) to filter out any keys from json_dict that are not expected by cls.__init__.
For classes that are not exclusively for input (e.g. those that we receive from TG), just using **kwargs is probably more effective. But we could use this logic for classes like BotCommandScope and InputMedia* as fail-safe. E.g. it is some maybe not highly effecient logic that can safe us if needed, but likely won't be called at all.

@Poolitzer
Copy link
Member

hm. so you want to fail then in the function if a keyword is unexpected, and you want to keep the kwargs parameter?

@Bibo-Joshi
Copy link
Member Author

No, I want to remove **kwargs and still make it work in case we actually do need to de-json one of the classes in question. Basically the Dismissal of additional kwargs section of #2698 (comment) :)

@Poolitzer
Copy link
Member

ah well that sounds like a pretty good approach actually.

@harshil21 harshil21 modified the milestones: v20, v20.0a1 May 12, 2022
@Bibo-Joshi Bibo-Joshi modified the milestones: v20.0a1, v20.0 May 21, 2022
@Bibo-Joshi
Copy link
Member Author

Closed by #3233

@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Development

No branches or pull requests

3 participants