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

PEP 747: TypeExpr: Type Hint for a Type Expression #3798

Merged
merged 47 commits into from
Jun 16, 2024

Conversation

davidfstr
Copy link
Contributor

@davidfstr davidfstr commented May 27, 2024

Also:

  • PEP 12: Explain how to use Intersphinx references. Mention that Typing docs can be referenced using them.
  • PEP 655: Remove gremlin character

CC sponsor @JelleZijlstra

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
    • DF: @JelleZijlstra has agreed to sponsor. Is that the approval required?
  • 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
    • DF: All code blocks are Python. Hopefully they are all PEP 8 compliant. I don't know how to check using a tool.
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
    • DF: There is a lint error on the Post-History line which I don't know how to fix
  • 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
    • [#] Open Issues
  • 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
    • DF: No endorsements included.
    • DF: However section §"Common kinds of functions that would benefit from TypeExpr" mentions several libraries that are expected to benefit from this PEP. Maintainers for all those libraries were tagged on a GitHub discussion thread. Only maintainer Hynek (of the attrs and svcs libraries) said there would be no benefit, and those libraries are no longer mentioned in this latest PEP draft.
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History
    • DF: After merging I plan to post to this existing thread, then make a small PR to link to the exact post.
    • DF: After merging I plan to post to a new thread, then make a small PR linking to the thread in both Discussions-To and Post-History.

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

Also:
* PEP 655: Remove gremlin character
@davidfstr davidfstr requested review from gvanrossum and a team as code owners May 27, 2024 19:54
.github/CODEOWNERS Outdated Show resolved Hide resolved
peps/pep-0755.rst Outdated Show resolved Hide resolved
peps/pep-0755.rst Outdated Show resolved Hide resolved
peps/pep-0755.rst Outdated Show resolved Hide resolved
peps/pep-0755.rst Outdated Show resolved Hide resolved
peps/pep-0755.rst Outdated Show resolved Hide resolved
@hugovk hugovk removed the request for review from gvanrossum May 27, 2024 20:43
@hugovk
Copy link
Member

hugovk commented May 27, 2024

  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
    • DF: @JelleZijlstra has agreed to sponsor. Is that the approval required?

Yes, Jelle will confirm in this PR.

  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
    • DF: There is a lint error on the Post-History line which I don't know how to fix

Discussions-To should be the new thread opened for the PEP draft. It also goes in Post-History. If you have further updates later which warrant a new thread, this is put in Discussions-To, and both threads are listed in Post-History. We don't need to list all the pre-PEP discussions.

  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

    • DF: After merging I plan to post to this existing thread, then make a small PR to link to the exact post.

Please open a new thread after the PEP is merged to the repo. That gives notice that there's a new PEP draft to be reviewed, and will make it more widely known: there are people not following that existing thread who will pay more attention to a new thread announcing a new PEP draft.

peps/pep-0755.rst Outdated Show resolved Hide resolved
davidfstr and others added 2 commits May 27, 2024 19:11
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
@davidfstr davidfstr changed the title PEP 755: TypeExpr: Type Hint for a Type Expression PEP 747: TypeExpr: Type Hint for a Type Expression May 27, 2024
@davidfstr davidfstr requested a review from CAM-Gerlach as a code owner May 28, 2024 14:32
@davidfstr
Copy link
Contributor Author

Feedback applied:

  • PEP number changed.
  • Post-History trimmed to point to draft 1 and draft 2 discussions only, and not any prior discussions.
  • Discussions-To removed until this draft 3 is shepherded to merge and a new discussion link for draft 3 is created.
  • Code formatting applied in many places.

One interesting thing I had to do was extend the linting infrastructure slightly to recognize discuss.python.org post links without a trailing slash (which is the format the web UI generates the links in by default).

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.

Thanks! This is a great writeup but I feel in places it goes into too much detail and specifies too many special cases.

peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented May 28, 2024

One interesting thing I had to do was extend the linting infrastructure slightly to recognize discuss.python.org post links without a trailing slash (which is the format the web UI generates the links in by default).

Previously a URL like the following was recognized:
https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435/7/
But the canonical URL actually has no trailing slash:
https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435/7
That canonical URL is now recognized.

The linter was working as expected: it's validating a <Thread URL>, which is meant to link to a discussion thread, not a message that's part of a thread.

It would happily accept thread URLs such as these, with or without a trailing slash:

  • https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435/
  • https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435

Please can you revert the linter change and link to a thread URL?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks for writing this PEP!

peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
@davidfstr
Copy link
Contributor Author

The linter was working as expected: it's validating a <Thread URL>, which is meant to link to a discussion thread, not a message that's part of a thread.

It would happily accept thread URLs such as these, with or without a trailing slash:

  • https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435/
  • https://discuss.python.org/t/typeform-spelling-for-a-type-annotation-object-at-runtime/51435

Please can you revert the linter change and link to a thread URL?

I can't change the URL of where draft 2 was introduced in the past, in the middle of an existing thread. Rounding that URL to point at the thread itself makes it more difficult to see where the draft was introduced, since the top of that thread is where draft 1 came from.

For now I've reverted the linter change and altered the draft 2 URL to point to the non-canonical URL that continues to reference exactly where draft 2 originated.

peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
@davidfstr
Copy link
Contributor Author

(I'm still integrating feedback, including larger feedback that involves creating/reorganizing entire sections. I expect to post a new major revision to this PR in the next few days, with a comment explaining major changes made.)

@davidfstr
Copy link
Contributor Author

davidfstr commented Jun 8, 2024

I've pushed a new draft of this PEP, with the following major changes:

  • Rationale: Section added. Explains what a "type expression" is, how it relates to similar concepts like "annotation expression" and "class object", and why TypeExpr was chosen to align with the definition of "type expression".
  • Specification > TypeExpr Values: Added specific rules for recognizing TypeExprs in value expression contexts, based on feedback from Eric. Added the explicit TypeExpr(...) syntax (with parentheses).
  • Specification: Deleted redundant subsections about Interactions with X, or moved/reintegrated these subsections as examples inside the How to Teach This supersection.
  • How to Teach This > Introspecting TypeExpr Values: Added section about writing the body of a function that takes TypeExpr as input. Clarified that TypeExpr is treated nearly the same as object.

I've also reverted the PEP 12 changes in this PR, in favor of moving those changes to a different PR, so brettcannon and warsaw shouldn't need to stay subscribed here.

@davidfstr
Copy link
Contributor Author

Friendly ping to @JelleZijlstra to take a look at some of the new major changes in this PR for TypeExpr

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.

Thanks, just a few small comments.

peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Show resolved Hide resolved
peps/pep-0747.rst Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <[email protected]>
@davidfstr
Copy link
Contributor Author

davidfstr commented Jun 16, 2024

(All feedback from Jelle has now either been applied or responded to)

@JelleZijlstra JelleZijlstra merged commit 3e200c6 into python:main Jun 16, 2024
6 checks passed
@davidfstr
Copy link
Contributor Author

Thanks @JelleZijlstra , @hugovk , and @AlexWaygood for providing feedback!

I'll create a new discuss.python.org thread for this PEP within the next day or two.

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.

5 participants