Skip to content
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

Add sort keys option for dataclasses inheriting from mixin #157

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

mishamsk
Copy link
Contributor

@mishamsk mishamsk commented Aug 30, 2023

Hi,

Thought this is small enough to directly open a PR and see if you would support this feature.

What: add a sort_keys option for BaseConfig which would efficiently generate serialization code such that keys are sorted on creation (which in many cases will make much heavier encoder approach unnecessary).

What's missing:

  • I've only added the feature to Mixin subclasses for now, but not for arbitrary dataclasses
  • I haven't added any docs, since there is no guarantee you would approve this PR
  • I've only added one test, maybe you would want additional ones

Rationale:
I am using key sort (via __post_serialize__ hook) a lot in order to maintain stable serialized view of my objects (those pesky trees). This would make it more efficient (depending on how classes are defined of course).

@Fatal1ty Fatal1ty self-requested a review September 1, 2023 12:16
Copy link
Owner

@Fatal1ty Fatal1ty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one minor but everything else is fine.

I've only added the feature to Mixin subclasses for now, but not for arbitrary dataclasses

It will also work for arbitrary dataclasses:

@dataclass
class Foo:
    b: int
    a: int

@dataclass
class Bar(DataClassDictMixin):
    z: int
    foo: Foo

bar = Bar(z=3, foo=Foo(b=2, a=1))
print(bar.to_dict())  # {'foo': {'a': 1, 'b': 2}, 'z': 3}

Would you like to add such a case to the tests since you mentioned it?

mashumaro/core/meta/code/builder.py Outdated Show resolved Hide resolved
@mishamsk
Copy link
Contributor Author

mishamsk commented Sep 9, 2023

Would you like to add such a case to the tests since you mentioned it?
forgot to pull latest changes, so naturally it didn't work for dataclasses in my branch.

now pulled and rebased. added a test + added a README

@Fatal1ty
Copy link
Owner

@mishamsk Thank you for contribution, I'm merging it now.

@Fatal1ty Fatal1ty merged commit 3780174 into Fatal1ty:master Sep 11, 2023
13 checks passed
@mishamsk
Copy link
Contributor Author

glad to help!

@Fatal1ty Fatal1ty mentioned this pull request Sep 13, 2023
@Fatal1ty
Copy link
Owner

@mishamsk

I just realised that I was wrong. Config options are not propagated to inner dataclasses by design, and the tests didn't show it because two dictionaries with different key order are equal. I created the pull request to fix the tests.

@mishamsk
Copy link
Contributor Author

@Fatal1ty ouch. sorry. my bad!

@mishamsk mishamsk deleted the add-sort-keys-option branch December 16, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants