Centralize de_json - #5186
Conversation
Some tests are still failing
All changes in this commit were done by Opus 4.6, some of which I'm not happy with, so that will be changed
aelkheir
left a comment
There was a problem hiding this comment.
Hey! This is really really neat.
I left a couple of suggestions and a questions for my understanding
| return lambda v, b, _c=item_type: ( # type: ignore[misc] | ||
| _c.de_list(v, b) if isinstance(v, list) else v | ||
| ) |
There was a problem hiding this comment.
| return lambda v, b, _c=item_type: ( # type: ignore[misc] | |
| _c.de_list(v, b) if isinstance(v, list) else v | |
| ) | |
| def transform_fn( | |
| value: object, | |
| bot: "Bot | None", | |
| tg_class: TelegramObject = item_type, | |
| ): | |
| if value is None: | |
| return () | |
| return ( # type: ignore[misc] | |
| tg_class.de_list(value, bot) if isinstance(value, list) else value | |
| ) | |
| return transform_fn |
the if value is None branch should not matter as parse_sequence_arg will be called on the value again. This is just to keep old behavior of de_list_optional
And btw way argumentparsing's de_list_optional, de_json_optional can be removed i think
There was a problem hiding this comment.
Thanks yeah the lambda's were not really readable..
I'm removing the value is None check because value is guaranteed to not be None (since we explicitly check that - see comment "Should we transform this field?")
| if not isinstance(data[key], dtm.datetime): # Avoid retransformations | ||
| data[key] = from_timestamp(data[key], tzinfo=tz) | ||
| elif isinstance(target, type): # Target is a TelegramObject subclass → de_json | ||
| if not isinstance(data[key], target): # Avoid retransformations |
There was a problem hiding this comment.
regarding the avoid retransformations comment, is this measure for classes that override de_json for whatever reason, or is there actually a another place that does pre transformations cause i couldn't find any
There was a problem hiding this comment.
Yeah that was actually to accommodate the de_json behaviour happening in PassportFile. The way the PassportFile de_json worked is flawed because the credentials object would already be a FileCredentials object in the de_json call, which is why I had to "avoid retransformations".
I'm now just going to fix the passport side instead to actually have dictionaries when doing a de_json, so we can delete this.
| target_cls: Tele_co = dispatch_mapping.get( # type: ignore[assignment] | ||
| data.get(dispatch_key) # type: ignore[arg-type] | ||
| ) | ||
| if target_cls is not None: |
There was a problem hiding this comment.
if for some reason the target_cls turned out to be None (for example telegram added a new child class to MessageOrigin) then continuing from this branch to the end of de_json would still produce a MessageOrigin with unknown fields added to api_kwargs?
There was a problem hiding this comment.
Yep that's right. Now that we have a lot of new classes not implemented, it's a perfect test for forward compatibility...
|
Did more forward compatibility tests with API 10.1/10.2, and they work as intended. Also added tests, which should explain the behavior of de_json at a higher level. |
|
A great undertaking, I dont have any open question. |
|
Alright, merging now! |
Closes #5189
This is the benchmark I used for the results below. It includes a mix of different kind of classes and cases.
Here are the performance results:
I'll add tests after initial review.