Movatterモバイル変換

[0]ホーム
URL:

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ambv merged 10 commits intopython:mainfromdebonte:descriptorTypedFieldTests
Jul 5, 2022

Conversation

debonte
Copy link
Contributor

@debontedebonte commentedJun 29, 2022•
edited by bedevere-bot
Loading

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

@bedevere-botbedevere-bot added awaiting review testsTests in the Lib/test dir labelsJun 29, 2022
Copy link
Contributor

@ambvambv 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:

classNDD:def__get__(self,obj,type):ifobjisNone:raiseAttributeErrorreturn"NDD Internal Value"@dataclasses.dataclassclassCls:ndd:NDD=NDD()obj=Cls(ndd="Something else!")print(obj.ndd)

This will print "Something else!" because setting the value will causendd to appear as a key inobj.__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 usingobj._x as internal storage for the data. This means that you cannot have two fields on the same dataclass of typeDescriptor 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
ContributorAuthor

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 anAttributeError when constructing a newInventoryItem.

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

@ambvambv added needs backport to 3.10only security fixes needs backport to 3.11only security fixes labelsJul 5, 2022
@ambvambv merged commit5f31930 intopython:mainJul 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 requestJul 5, 2022
…ythonGH-94424)Co-authored-by: Łukasz Langa <lukasz@langa.pl>(cherry picked from commit5f31930)Co-authored-by: Erik De Bonte <erikd@microsoft.com>
@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelJul 5, 2022
@bedevere-bot
Copy link

GH-94576 is a backport of this pull request to the3.11 branch.

@miss-islington
Copy link
Contributor

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

ambv pushed a commit to ambv/cpython that referenced this pull requestJul 5, 2022
…fields (pythonGH-94424)Co-authored-by: Łukasz Langa <lukasz@langa.pl>(cherry picked from commit5f31930)Co-authored-by: Erik De Bonte <erikd@microsoft.com>
@bedevere-bot
Copy link

GH-94577 is a backport of this pull request to the3.10 branch.

ambv added a commit that referenced this pull requestJul 5, 2022
) (GH-94576)Co-authored-by: Erik De Bonte <erikd@microsoft.com>Co-authored-by: Łukasz Langa <lukasz@langa.pl>(cherry picked from commit5f31930)
ambv added a commit that referenced this pull requestJul 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 commit5f31930)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ambvambvambv requested changes

@ericvsmithericvsmithAwaiting requested review from ericvsmithericvsmith is a code owner

Assignees

@ambvambv

Labels
testsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@debonte@ambv@miss-islington@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp