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 705: Simplify and clarify proposal #3504

Merged
merged 8 commits into from
Oct 24, 2023

Conversation

alicederyn
Copy link
Contributor

@alicederyn alicederyn commented Oct 21, 2023

Address feedback from @erictraut, plus fix issues I spotted in the process:

  • readonly=True appears to be more confusing than expected, and is not strictly necessary, so remove it (see Rejected Alternatives for more)
  • The landscape has changed around type discrimination, making it less important to address as part of this PEP, so this is fully deferred to PEP-728 (again, see Rejected Alternatives for more)
  • The Python docs use "item" not "entry", so standardize on that term
  • There is no explicit name given in the Python docs for d1 | d2, but "merge" is clearer than "union", which is used for sets
  • The merge section incorrectly required consistency between A and C, even if the value in A could never end up in C due to it being required in B
  • The merge section incorrectly allowed keys to be missed off A and B in a way that could allow unsound typing
  • As other_keys is no longer an option for safe updates to TypedDicts with read-only entries, highlight that updates are safe if the bottom type is used to explicitly exclude a key, and require type checker support
  • copy and deepcopy were mentioned as having similar behaviour to merge, but it was not clear if this was intended to change how type checkers behave; add a new section explicitly laying out how copy and deepcopy should work for TypedDicts, so this can be discussed
  • 2023-10-23 The "union operation" section has been removed completely after discussion on this PR (see Rejected Alternatives)

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

@alicederyn alicederyn requested a review from pablogsal as a code owner October 21, 2023 13:47
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

Current changes look good to me — at least sufficient for getting another round of feedback from the broader typing community.

peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
I believe the typeshed is not *unsound* here, merely stricter than necessary. Consensus is that PEPs should dictate the typeshed only, and I think this is best left to a separate PEP.

Typecheckers are always permitted to widen what they support beyond what the typeshed dictates, e.g. to specify that `c: C = a | b` and `d: D = copy(a)` should match the equivalent expressions `c: C = {**a, **b}` and `d: D = {**a}`
@alicederyn alicederyn requested a review from erictraut October 23, 2023 08:23
@alicederyn
Copy link
Contributor Author

alicederyn commented Oct 23, 2023

Re-requesting review as I have made significant changes: the sections on merge and copy/deepcopy have been removed, with explanation on Rejected Alternatives.

Copy link
Contributor

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

I added a couple of minor comments, but I think this draft is looking really good!

peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
@alicederyn
Copy link
Contributor Author

Admin question: when this merges, should I continue to use the existing discussion thread on typing-sig, and just post an update? Or should I open a new one?

@JelleZijlstra
Copy link
Member

Admin question: when this merges, should I continue to use the existing discussion thread on typing-sig, and just post an update? Or should I open a new one?

I think you should continue the existing thread. Not sure we have firm guidelines, but I would only start a new thread if the proposal becomes radically different or the thread has become very long.

peps/pep-0705.rst Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra merged commit 718157a into python:main Oct 24, 2023
4 checks passed
@alicederyn alicederyn deleted the simplify.pep.705 branch October 25, 2023 15:56
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.

3 participants