GeezerVison is showing github.com — gh-119180: Documentation for PEP 649 and 749 by JelleZijlstra · Pull Request #122235 · python/cpython
Conversation
JelleZijlstra
commented
Jul 24, 2024
•
edited by github-actions
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't look at everything nor had I marked every typo but here's a first round of comments.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a few comments.
General points -- the line lengths in the new text seem a bit long. Also, I wonder if it is worth covering / providing call-outs to the interactions with the annotations future? I imagine that will be something many readers will be thinking about.
A
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
119
to
+122
|
|
``from __future__ import annotations`` was previously scheduled to
|
|
|
become mandatory in Python 3.10, but the Python Steering Council
|
|
|
twice decided to delay the change
|
|
|
(`announcement for Python 3.10 <https://mail.python.org/archives/list/[email protected]/message/CLVXXPQ2T2LQ5MP2Y53VVQFCXYWQJHKZ/>`__;
|
|
|
`announcement for Python 3.11 <https://mail.python.org/archives/list/[email protected]/message/VIZEBX5EYMSYIJNDBF6DMUMZOCWHARSO/>`__).
|
|
|
No final decision has been made yet. See also :pep:`563` and :pep:`649`.
|
|
|
become mandatory in Python 3.10, but the change was delayed and ultimately
|
|
|
canceled. This feature will eventually be deprecated and removed. See
|
|
|
:pep:`649` and :pep:`749`.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide or link to migration guidance here, especially given that this is the first time a __future__ won't become the actual future?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+17
to
+18
|
|
The :mod:`!annotationlib` module provides tools for introspecting :term:`annotations <annotation>`
|
|
|
on modules, classes, and functions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially this could be added later, but I think a longer narrative summary of annotationlib and where it fits in could be of use -- we jump straight into functions here. Contrast with pathlib, heapq, hashlib.
I wonder if there are words from 749 that we could incorporate? Effectively, something that briefly talks to where this module fits into the landscape, perhaps aimed at the fairly experienced user of types/inspect/typing, but someone who hasn't followed the PEPs.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of adding that too, but ran out of energy while working on the initial version. :) I'll probably work on this later.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
AA-Turner reacted with thumbs up emoji
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote such a summary now, along with a historical account of annotation semantics, since that seems like something the docs should cover.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Doctest failures:
Document: library/annotationlib
-------------------------------
**********************************************************************
File "library/annotationlib.rst", line 67, in default
Failed example:
call_evaluate_function(Alias.evaluate_value, Format.VALUE)
Exception raised:
Traceback (most recent call last):
File "/home/runner/work/cpython/cpython/Lib/doctest.py", line 1395, in __run
exec(compile(example.source, filename, "single",
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
compileflags, True), test.globs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<doctest default[1]>", line 1, in <module>
call_evaluate_function(Alias.evaluate_value, Format.VALUE)
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/cpython/cpython/Lib/annotationlib.py", line 421, in call_evaluate_function
return call_annotate_function(evaluate, format, owner=owner, _is_evaluate=True)
File "/home/runner/work/cpython/cpython/Lib/annotationlib.py", line 446, in call_annotate_function
return annotate(format)
File "<doctest default[0]>", line 1, in Alias
type Alias = undefined
^^^^^^^^^
NameError: name 'undefined' is not defined
**********************************************************************
Document: library/typing
------------------------
**********************************************************************
File "library/typing.rst", line 2181, in default
Failed example:
Alias.__value__
Exception raised:
Traceback (most recent call last):
File "/home/runner/work/cpython/cpython/Lib/doctest.py", line 1395, in __run
exec(compile(example.source, filename, "single",
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
compileflags, True), test.globs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<doctest default[1]>", line 1, in <module>
Alias.__value__
File "<doctest default[0]>", line 1, in Alias
type Alias = undefined
^^^^^^^^^
NameError: name 'undefined' is not defined
**********************************************************************
File "library/typing.rst", line 2185, in default
Failed example:
Alias.evaluate_value(Format.VALUE)
Exception raised:
Traceback (most recent call last):
File "/home/runner/work/cpython/cpython/Lib/doctest.py", line 1395, in __run
exec(compile(example.source, filename, "single",
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
compileflags, True), test.globs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<doctest default[3]>", line 1, in <module>
Alias.evaluate_value(Format.VALUE)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
File "<doctest default[0]>", line 1, in Alias
type Alias = undefined
^^^^^^^^^
NameError: name 'undefined' is not defined
**********************************************************************
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
savannahostrowski
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these are nits. I got curious about the PR and found some small things 🙈 !
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good @JelleZijlstra. I'm wondering if we are going a bit too deep into implementation in the Data Model Language Reference page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks for the review @willingc! Which part do you think goes into too much detail? I looked over the data model changes and all the changes seem relevant to me.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Thanks for the review @willingc! Which part do you think goes into too much detail? I looked over the data model changes and all the changes seem relevant to me.
@JelleZijlstra I reread the data model changes again on the rendered page (instead of the diff). On the initial read, I thought there was implementation content mixed in, but I was mistaken and misled by the rST markup. Thanks for rechecking and sorry for the extra noise.
Let's 🚢 this. 😄
|
JelleZijlstra reacted with rocket emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|