Skip to content

Conversation

@thejcannon
Copy link
Contributor

@thejcannon thejcannon commented Apr 10, 2023

FYI @ericvsmith


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

@thejcannon thejcannon requested a review from a team as a code owner April 10, 2023 01:30
@AA-Turner AA-Turner changed the title PEP 9999: Adding "converter" dataclasses field specifier parameter PEP 712: Adding "converter" dataclasses field specifier parameter Apr 10, 2023
@CAM-Gerlach CAM-Gerlach added the new-pep A new draft PEP submitted for initial review label Apr 11, 2023
@CAM-Gerlach CAM-Gerlach self-requested a review April 11, 2023 00:49
@CAM-Gerlach
Copy link
Member

Just a reminder, before making further changes locally, I suggest you directly apply all the suggestions you want in one go by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit.

Also, all of the checklist items should be addressed by either suggestions or at least review comments, except for the three suggested sections that are not present. My recommendations for them:

  • In the Backward Compatibility section, mention any backward compatibility implications of the new parameter, attribute, etc. to existing working user code, libraries and type checkers. If there are none, simply state this and the reason why (new parameter/attribute, etc). Would be good to at least briefly discuss with your Sponsor (@ericvsmith ), who is likely to have a better grasp of this.

  • In the Security Implications section, mention any impacts these new features might have on security (positive or negative, e.g. fixes a vulnerability, opens up potential for malicious use, allows for tools to better perform security checks, etc). If there are no effects either way, then simply state "There are no known security implications of these changes.", potentially with a brief reason. Again, would be good to ask your Sponsor about this if you're unsure.

  • In the How to Teach This section, at least briefly mention or give an example of how you'll explain this to users, if it is a user-facing feature. E.g. what will be added to the Python or Typing docs? How might the chance be publicly communicated? How might you tell a user how to use the user-facing portions of it?

@hugovk
Copy link
Member

hugovk commented Apr 11, 2023

@thejcannon Hi! Suggestion: as you're a new contributor to this repo, would you like to pick one file that has a typo from here and make a PR to fix it? We can then merge it right away, and then we won't need to click "Approve and run" for the CI here each time as we iterate through review rounds :)

@thejcannon
Copy link
Contributor Author

@CAM-Gerlach thanks for your time and the thorough review, especially with suggestions. 🙌

@thejcannon thejcannon mentioned this pull request Apr 11, 2023
@thejcannon
Copy link
Contributor Author

@thejcannon Hi! Suggestion: as you're a new contributor to this repo, would you like to pick one file that has a typo from here and make a PR to fix it? We can then merge it right away, and then we won't need to click "Approve and run" for the CI here each time as we iterate through review rounds :)

#3099

@thejcannon thejcannon changed the title PEP 712: Adding "converter" dataclasses field specifier parameter PEP 712: Adding a "converter" parameter to dataclasses.field Apr 11, 2023
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Various followup fixes, thanks. This is starting to look pretty good from a PEP editor point of view, (though it'll need further review on the technical merits by your sponsor before merging of course).

Just FYI, as mentioned previously, you can directly apply all the suggestions you want in one go automatically by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit. This saves you time manually replicating them yourself, and us time reviewing that they were applied correctly.

@CAM-Gerlach CAM-Gerlach requested a review from ericvsmith April 12, 2023 02:11
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

A couple minor suggestions on the followup changes, and re-iterating one previous suggestion that was not applied, mistakenly manually resolved and is still resulting in a build warning.

Also, not sure where my checklist went; my entire initial review comment seems to have completely disappeared (and its not even possible for to delete such even intentionally, as far as I can tell).

Reminder: You can directly apply all the suggestions you want in one go by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

One tiny nit, otherwise LGTM from a PEP editor perspective bar the things your sponsor (and anyone else concerned) should review (and of course any other comments they have).

Since somehow it seems GitHub mysteriously deleted the original checklist after I'd posted and updated at some length, here it is for the record:

Basic requirements (all PEP Types)

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

Standards Track requirements

  • PEP topic discussed in a suitable venue with general agreement that a PEP is appropriate
  • Suggested sections included (unless not applicable)
    • Motivation
    • Rationale
    • Specification
    • Backwards Compatibility
    • Security Implications
    • How to Teach This
    • Reference Implementation
    • Rejected Ideas
  • Python-Version set to valid (pre-beta) future Python version, if relevant
  • Any project stated in the PEP as supporting/endorsing/benefiting from the PEP formally confirmed such
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

@CAM-Gerlach CAM-Gerlach dismissed their stale review April 14, 2023 02:54

All major changes addressed

@ericvsmith
Copy link
Member

I'm traveling and using my phone, so it's much easier for me if this shows up on peps.python.org before I review it, so I'll wait until it shows up there.

@thejcannon
Copy link
Contributor Author

If you wanted to, you could use the documentation preview: (direct link) https://pep-previews--3095.org.readthedocs.build/pep-0712/

@ericvsmith
Copy link
Member

@thejcannon : that's good to know, thanks!

@thejcannon
Copy link
Contributor Author

No worries. Looks like CI updates the PR description with a link to the preview docs containing the changes.

That's how I found it

@thejcannon
Copy link
Contributor Author

Also, Thanks again @CAM-Gerlach for your feedback and patience. It is very much appreciated.

Copy link
Member

@JelleZijlstra JelleZijlstra 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 a few comments.

Also, this introduces some fairly subtle behavior around typing, so it would be good to submit the PEP to the typing-sig email list for typing-specific feedback.

@debonte
Copy link
Contributor

debonte commented Apr 21, 2023

Thanks for your work on this @thejcannon!

@erictraut may want to review this as well.

thejcannon and others added 2 commits April 22, 2023 13:30
Co-authored-by: Erik De Bonte <erikd@microsoft.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@thejcannon
Copy link
Contributor Author

Thank you sincerely @debonte and @erictraut , the suggested wording and feedback has made this look MUCH nicer and simpler.

...and sorry for what this means for type-checkers 😂

@JelleZijlstra JelleZijlstra merged commit 468f3ed into python:main Apr 23, 2023
@thejcannon thejcannon deleted the converter branch April 23, 2023 00:15
@ericvsmith
Copy link
Member

@thejcannon I’d just open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-pep A new draft PEP submitted for initial review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants