-
-
Notifications
You must be signed in to change notification settings - Fork 50
Comments
Apply dataclass transform to AtomMeta#210
Conversation
frmdstryr
commented
Mar 19, 2024
I still use the "old" method of just using members directly but this looks fine to me.
0c93f70 to
3f96820
Compare
Codecov Report
Attention: Patch coverage is 83.68794% with 23 lines in your changes missing coverage. Please review.
Project coverage is 95.93%. Comparing base (
60eca49) to head (d62b781).
Report is 2 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@ ## main #210 +/- ## ========================================== - Coverage 97.67% 95.93% -1.75% ========================================== Files 24 24 Lines 1074 1180 +106 Branches 162 186 +24 ========================================== + Hits 1049 1132 +83 - Misses 12 29 +17 - Partials 13 19 +6
sccolbert
commented
Jan 20, 2025
I don't think I'm following what you want here. Could you please explain a bit more with the some code examples of what you are trying to achieve?
MatthieuDartiailh
commented
Jan 21, 2025
My goal is overall to make Atom feel closer to other technology such as the stdlib dataclass. A first step that will improve the interaction with IDE/type checker is to use the dataclass_transform on the atom metaclass: https://typing.readthedocs.io/en/latest/spec/dataclasses.html#the-dataclass-transform-decorator
In order to achieve that I want to never use the Member subclasses explicitly and only use type annotations and field specifiers (https://typing.readthedocs.io/en/latest/spec/dataclasses.html#field-specifiers).
sccolbert
commented
Jan 21, 2025
I think this would be a great change for the next version of Atom where we can introduce some breaking changes. That would let us cleanup/remove the members that don't make sense, while also simplifying the C++ code base.
For example, we could have just one Member class behind the scenes which has an enum defining its type. That type could be type-checked when running in a "debug" mode, but be cost-free when running in production.
sccolbert
commented
Jan 21, 2025
Another thing I want to test is the space/perf efficiency of the new(er) Python dict implementation for instance attributes. I've read that it's quite an improvement over Python 2.x, but I've been wondering how they compare to Atom for space and performance.
MatthieuDartiailh
commented
Jan 22, 2025
Another thing I want to test is the space/perf efficiency of the new(er) Python dict implementation for instance attributes. I've read that it's quite an improvement over Python 2.x, but I've been wondering how they compare to Atom for space and performance.
Size wise the last I did the comparison an Atom instance was still smaller than a Python class but larger than a slotted dataclass. Speed wise I need to redo some benchmarks.
MatthieuDartiailh
commented
Jan 22, 2025
I think this would be a great change for the next version of
Atomwhere we can introduce some breaking changes. That would let us cleanup/remove the members that don't make sense, while also simplifying the C++ code base.
If I reduce the scope of the PR to simply use dataclass_transform and make Member, its and set_default fields, it would already be a nice improvement (we would get autocompletion of the metaclass kwargs I think). Given the time I manage to put into open source it is a much safer bet.
For example, we could have just one
Memberclass behind the scenes which has an enum defining its type. That type could be type-checked when running in a "debug" mode, but be cost-free when running in production.
I am not sure I get the enum idea. It may be worth putting in the discussion I started or a dedicated issue. To me a Member is not uniquely defined, it is the modes that make it what it is and I think we should be checking against those rather than a type.
All members are declared as field specifiers.
... to set more default modes and metadata
8b8e513 to
337ef33
Compare
All members are declared as field specifiers which allow to use them in addition of an annotation.
I am considering adding other field specifiers to:
I am considering something along the following lines to stay close to the existing syntax.
Any opinion @frmdstryr
Closes #173