Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

Apply dataclass transform to AtomMeta#210

Open
MatthieuDartiailh wants to merge 14 commits intomain from
dataclass-transform
Open

Apply dataclass transform to AtomMeta #210
MatthieuDartiailh wants to merge 14 commits intomain from
dataclass-transform

Conversation

@MatthieuDartiailh
Copy link
Member

@MatthieuDartiailh MatthieuDartiailh commented Mar 18, 2024

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:

  • allow to tag a member without needing to rely on a member
  • allow to specify a default value factory
  • I would like to be able to specify arg/kwargs for Typed/Instance but this cannot be statically validated...

I am considering something along the following lines to stay close to the existing syntax.

class A(Atom):
 a: int = member(default=1).tag(a=1)
 

Any opinion @frmdstryr

Closes #173

Copy link
Contributor

I still use the "old" method of just using members directly but this looks fine to me.

Copy link

codecov bot commented Mar 25, 2024
edited
Loading

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 

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate support for PEP681 : dataclass_transform

AltStyle によって変換されたページ (->オリジナル) /