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 728: Improve specification #4166

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Dec 13, 2024

  • Specify that extra_items and closed are also supported with the
    functional syntax.
  • Rewrite the rules for closed=True and inheritance. I attempted
    to make closed=True exactly equivalent to extra_items=Never in
    terms of inheritance. The semantics as specified in the previous
    version of the PEP felt harder to understand and less consistent.
  • Fix some incorrect comments regarding expected type checker errors.
  • Clarify section on assignability with Mapping
  • Add section on runtime behavior. I tried to make the intended runtime
    behavior simple to implement and understand. This makes the runtime
    simpler but may make life more complicated for tools consuming the
    metadata.
  • Clean up trailing whitespace

This grew out of work on python/typing-extensions#519. cc @PIG208


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

- Specify that extra_items and closed are also supported with the
  functional syntax.
- Rewrite the rules for `closed=True` and inheritance. I attempted
  to make `closed=True` exactly equivalent to `extra_items=Never` in
  terms of inheritance. The semantics as specified in the previous
  version of the PEP felt harder to understand and less consistent.
- Fix some incorrect comments regarding expected type checker errors.
- Clarify section on assignability with Mapping
- Add section on runtime behavior. I tried to make the intended runtime
  behavior simple to implement and understand. This makes the runtime
  simpler but may make life more complicated for tools consuming the
  metadata.
Copy link
Contributor

@PIG208 PIG208 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 the update! Just have one comment on this.

peps/pep-0728.rst Outdated Show resolved Hide resolved
adapted_from_novel: bool

class MovieRequiredYear(MovieBase): # Not OK. Required key 'year' is not known to 'Parent'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class MovieRequiredYear(MovieBase): # Not OK. Required key 'year' is not known to 'Parent'
class MovieRequiredYear(MovieBase): # Not OK. Required key 'year' is not known to 'MovieBase'

class AdaptedMovie(MovieBase): # Not OK. 'bool' is not assignable to 'int | None'

class AdaptedMovie(MovieBase): # Not OK. 'bool' is required
Copy link
Contributor

Choose a reason for hiding this comment

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

AdaptedMovie and MovieRequiredYear seem redundant with each other now. They're both examples of a required key.

@@ -384,14 +396,14 @@ For example::

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is preexisting, but there's a typo three lines above (misspelling of overridden).

(Sadly, I can't comment on the exact line - https://github.com/orgs/community/discussions/4452.)

Copy link
Member

Choose a reason for hiding this comment

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

And another:

make spellcheck
...
peps/pep-0728.rst:393: overriden ==> overridden
peps/pep-0728.rst:706: othere ==> other

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