GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Switching to Dataclasses by aelkheir · Pull Request #5334 · python-telegram-bot/python-telegram-bot Conversation
|
|
So I had this drafted for a bit of time now, I intended to send it earlier on to get your feedback on it, especially the logic around the after-processing of generated Then I was tempted to walk till (near) the finish line and see how (simple v.s complicated) it will turn to be.
This did not age well. from dataclasses import InitVar, dataclass, field @dataclass(frozen=True) class Example: value: InitVar[int | None] = None _value: int | None = field(init=False, default=None) def __post_init__(self, value: int | None, /): object.__setattr__(self, "_value", value) @property def value(self) -> int | None: print("value is deprecated and will be removed") return self._value a = Example() print(a._value) # output: <property object at 0x7ff25e629e40> b = Example(10) print(b._value) # output: 10 When not providing an explicit argument for value, the default So i tried giving this alias Field specifier parameters a shot:
The outcome of it is pretty neat and less verbose, however, and just like Aliased parameter MWE# No runtime support fo alias, this is just to demonstrate type checkers support from dataclasses import MISSING, dataclass, field from typing import Any, TypeVar, dataclass_transform _T = TypeVar("_T") def tg_field( *, default: Any = MISSING, alias: str | None = None, ) -> Any: return field(default=default) @dataclass_transform(field_specifiers=(tg_field,)) def tg_dataclass(cls: type[_T]) -> type[_T]: return dataclass(frozen=True)(cls) @tg_dataclass class Example: _value: int | None = tg_field(alias="value", default=None) @property def value(self) -> int | None: print("value is deprecated and will be removed") return self._value # Wroks in runtime c = Example(_value=10) # TypeError: Example.__init__() got an unexpected keyword argument 'value' d = Example(value=10) Its well supported in type checkers that common IDEs use IDE SupportPyright
Pycharm, should be supported from 2024.3 onwards. https://youtrack.jetbrains.com/issue/PY-73832/Pycharm-doesnt-properly-recognizes-aliases-for-Pydantic-dataclass mypy also support aliases running it through the above MWE gives:
I just did nothing. Excluding test files, there was very minimal unfreezing of objects outside of old CISadly pylint and mypy have a bunch of false positives that we'll have to fight some way. I still got some polishing to do, specifically I want to:
The last two points can further simplify I'm going to be working on these items in the following days, would be great to get some initial review since Doesn't have to be the whole thing, just |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
First round of reviews! Overall, I'm pretty happy with the implementation so far. I only looked at TGO.py and dataclasses.py in this round.
Experiment with no longer tampering with the generated init signatures and annotations and instead make TO._de_json() dataclasses-first,
I would probably leave this for a future PR (in the interest of time), I think we have right now can work fine.
One thing I would double check is if backward / forward compatibility isn't affected by this. I know we have tests, but can't hurt to check if they work with these changes..
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| - Direct instances of :class:`~telegram.TelegramObject` are frozen by default. | ||
| - Loading a pickled :class:`~telegram.TelegramObject` will return a frozen instance by default. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This was already existing behaviour?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
True for subclasses
But if you initialize a TelegramObject directly it used to come unfrozen.
We had a comment there saying it was to make subclasses not have to call unfrozen in every single one of their inits
For pickling, previously unpickling a (unfrozen when pickled) object would load it unfozen just like it was. Now it will be loaded frozen regardless.
Thanks I'll now rephrase that second point i can see it can be unclear
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| """Special marker indicating that an entry was deleted.""" | ||
|
|
||
|
|
||
| def to_sequence_annotation(annotation: object) -> object: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
a docstring showing why this is needed would be nice
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| from telegram._utils.de_json import unwrap_optional | ||
|
|
||
| _T = TypeVar("_T") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
we can use the Self type now that we're dropping 3.10 support
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return cls | ||
|
|
||
| aliased_init.__signature__ = signature # type: ignore[attr-defined] | ||
| cls.__init__ = aliased_init # type: ignore[method-assign] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this means that during initialization, the for loop of aliased_init is going to run every time checking every field. If we replace has_aliases with:
alias_pairs = tuple(
(dataclass_field.name, alias)
for dataclass_field in fields.values()
if dataclass_field.init
and (alias := dataclass_field.metadata.get(ALIAS_KEY)) is not None
)
and then just do the for loop with the alias pairs then that should be slightly faster.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| def tg_dataclass( | ||
| *, | ||
| eq: bool = True, | ||
| ) -> Callable[[type[_T]], type[_T]]: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I would put a docstring for this and tg_field
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| for dataclass_field in dataclasses.fields(self): | ||
| converter = dataclass_field.metadata.get(CONVERTER_KEY) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
we can do a similar thing here like my previous comment about aliases: cache the fields which need a converter and only iterate over those, instead of every field
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| if TYPE_CHECKING: | ||
| # FIXME: not sure what to do about this | ||
| # mypy doesn't recognize converters currently |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
note to self: it's time to migrate to ty and drop mypy
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Switching to Dataclasses
There was an error while loading. Please reload this page.