Skip to content

Conversation

@thejcannon
Copy link
Contributor

@thejcannon thejcannon commented May 22, 2023

  • Change is either:
    • To a Draft PEP
    • To an Accepted or Final PEP, with Steering Council approval
    • To fix an editorial issue (markup, typo, link, header, etc)
  • PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

📚 Documentation preview 📚: https://pep-previews--3152.org.readthedocs.build/

@thejcannon thejcannon requested a review from ericvsmith as a code owner May 22, 2023 19:13
@thejcannon
Copy link
Contributor Author

My fork of CPython was also updated with the semantics changes.

FYI @erictraut and @debonte, I'll make an issue in https://github.com/microsoft/pyright for the new semantics (primarily needing to type-check default_factory and attribute assignment)

@debonte
Copy link
Contributor

debonte commented May 22, 2023

I'll make an issue in https://github.com/microsoft/pyright for the new semantics (primarily needing to type-check default_factory and attribute assignment)

It looks like Pyright is already validating that default and the return type of default_factory match the input type of converter. Do you have some scenarios where this doesn't work?

@thejcannon
Copy link
Contributor Author

I naively assumed that because the PEP didn't mention default_factory it wasn't supported yet 😅 I'll play around and open a ticket for attribute assignment, if necessary.

@thejcannon
Copy link
Contributor Author

@CAM-Gerlach sorry to ping, but what's the protocol for merging an update PR? I assume I wait for an editor's suggestions?

Also @ericvsmith I assume this about does it in terms of solidifying the behavior. When you get time, please read through and let me know what you suggest for next steps.

Thanks to you both 😄

@JelleZijlstra
Copy link
Member

We'd generally want approval from the sponsor of the PEP before merging.

@thejcannon
Copy link
Contributor Author

@ericvsmith gentle bump 🙂

Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

I can't add review comments to lines that are already merged, so I'll add a few nits here.

Line 23 "A common feature these libraries provide ..." could use the word "other" before "libraries", since the previous sentence also refers to dataclasses.

Somewhere in the PEP you should say that these conversions are one-way: on setting attributes, not when reading them.

My main concern with the PEP has always been that there's no way to immediately see what the type of a field really is. You have to know the type returned by the converter. I'd like to see this point explicitly mentioned somewhere.

@thejcannon
Copy link
Contributor Author

@ericvsmith, thanks for taking a look and providing feedback 🙌

My main concern with the PEP has always been that there's no way to immediately see what the type of a field really is. You have to know the type returned by the converter. I'd like to see this point explicitly mentioned somewhere.

I'm not sure exactly what you mean. The type of the field is still explicitly laid out when the dataclass is defined:

@dataclass
class MyDataclass:
    # a is of type `str` no matter what
    a: str = field(converter=whatever)

@ericvsmith
Copy link
Member

@ericvsmith, thanks for taking a look and providing feedback 🙌

My main concern with the PEP has always been that there's no way to immediately see what the type of a field really is. You have to know the type returned by the converter. I'd like to see this point explicitly mentioned somewhere.

I'm not sure exactly what you mean. The type of the field is still explicitly laid out when the dataclass is defined:

@dataclass
class MyDataclass:
    # a is of type `str` no matter what
    a: str = field(converter=whatever)

Oops, I meant the type of the thing you can pass in to the constructor. You have to know what whatever takes as its parameter.

@thejcannon
Copy link
Contributor Author

Oops, I meant the type of the thing you can pass in to the constructor. You have to know what whatever takes as its parameter.

I suspected, but didn't want to assume. And yeah that's fair.

@thejcannon
Copy link
Contributor Author

OK, I added a section: "Indirection of allowable argument types". Take a look and let me know what you think.

FWIW I tried the feature on pyright, and saw:
Screenshot from 2023-06-15 10-03-45

Screenshot from 2023-06-15 10-20-55

str probably wasn't the best choice, but you get the idea 😅

@thejcannon
Copy link
Contributor Author

@ericvsmith please take another look.

I don't foresee much additional changing so let me know what the appropriate next step is, from your experience 🙂

@thejcannon
Copy link
Contributor Author

Friendly bump @ericvsmith 😄


I also realized that this takes effect for replace, but when I re-read-through I wasn't convinced the text needed a change since the wording already should handle replace.

@ericvsmith
Copy link
Member

I think this version is good.

@thejcannon
Copy link
Contributor Author

Ready for merge 🚀 (And submission to SC)

@AA-Turner AA-Turner merged commit 333b297 into python:main Aug 16, 2023
@AA-Turner
Copy link
Member

Thanks Josh, the next step would be to work with Eric to submit the PEP for approval.

A

@thejcannon thejcannon deleted the pep712 branch August 16, 2023 20:24
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.

6 participants