Skip to content

gh-91330: Tests and docs for dataclass descriptor-typed fields #94424

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

Merged
merged 10 commits into from
Jul 5, 2022

Conversation

debonte
Copy link
Contributor

@debonte debonte commented Jun 29, 2022

Add unit tests and documentation covering the behaviors of descriptor-typed fields on dataclasses.

@debonte debonte requested a review from ericvsmith as a code owner June 29, 2022 16:49
@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Jun 29, 2022
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

I left some smaller-scale comments inline but there's a bigger omission in this change: what you are describing only works for data descriptors. Consider this non-data descriptor (doesn't define either __set__ or __delete__) example:

class NDD:
    def __get__(self, obj, type):
        if obj is None:
            raise AttributeError

        return "NDD Internal Value"


@dataclasses.dataclass
class Cls:
    ndd: NDD = NDD()


obj = Cls(ndd="Something else!")
print(obj.ndd)

This will print "Something else!" because setting the value will cause ndd to appear as a key in obj.__dict__.

While non-data descriptors have little value as dataclass fields, I think it's worth pointing out explicitly that we're talking about data descriptors.

Another thing that irks me is that the example is using obj._x as internal storage for the data. This means that you cannot have two fields on the same dataclass of type Descriptor because they will overwrite each other's internal storage. (On the other hand storing state on the object allows it to be trivially picklable.)

Maybe you can come up with a more realistic example. We could also use an example of setting and retrieving a value, both of which would trigger the descriptor. This is currently missing.

@debonte
Copy link
Contributor Author

debonte commented Jun 29, 2022

Thanks for your feedback @ambv!

I think it's worth pointing out explicitly that we're talking about data descriptors.

It looks like it only works properly if you have both __get__ and __set__. Is there a term for that? Data descriptor isn't quite right. If you have __get__ and __delete__ but no __set__ you'll get an AttributeError when constructing a new InventoryItem.

Another thing that irks me is that the example is using obj._x as internal storage for the data.

Fixed to leverage __set_name__

Maybe you can come up with a more realistic example.

I changed the descriptor to convert inputs to int. This doesn't add much code, but it's easy to tell that __set__ is actually being called from the examples. Is that realistic enough?

We could also use an example of setting and retrieving a value, both of which would trigger the descriptor. This is currently missing.

Fixed

@ambv
Copy link
Contributor

ambv commented Jul 5, 2022

Beautiful! I love the current example. Short but realistically scalable to non-trivial use cases. And includes __set_name__ as a best practice. Thanks for this improvement.

@ambv ambv added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jul 5, 2022
@ambv ambv merged commit 5f31930 into python:main Jul 5, 2022
@miss-islington
Copy link
Contributor

Thanks @debonte for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 5, 2022
…ythonGH-94424)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 5f31930)

Co-authored-by: Erik De Bonte <erikd@microsoft.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 5, 2022
@bedevere-bot
Copy link

GH-94576 is a backport of this pull request to the 3.11 branch.

@miss-islington
Copy link
Contributor

Sorry, @debonte and @ambv, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5f319308a820f49fec66fc3ade50bbaa9fe2105d 3.10

ambv pushed a commit to ambv/cpython that referenced this pull request Jul 5, 2022
…fields (pythonGH-94424)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 5f31930)

Co-authored-by: Erik De Bonte <erikd@microsoft.com>
@bedevere-bot
Copy link

GH-94577 is a backport of this pull request to the 3.10 branch.

ambv added a commit that referenced this pull request Jul 5, 2022
) (GH-94576)

Co-authored-by: Erik De Bonte <erikd@microsoft.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 5f31930)
ambv added a commit that referenced this pull request Jul 5, 2022
…GH-94424) (GH-94577)

Co-authored-by: Erik De Bonte <erikd@microsoft.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 5f31930)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants