-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Initial import of PEP 593: Flexible function and variable annotations #1014
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
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
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.
Thanks! This is not ready yet to be merged, I have a bunch of comments here.
pep-0592.rst
Outdated
Content-Type: text/x-rst | ||
Created: 26-April-2019 | ||
Python-Version: | ||
Post-History: |
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.
Please add Discussion-to:
header pointing to typing-sig
mailing list.
pep-0592.rst
Outdated
effectively crowded out any other form of annotation. Some of the use | ||
cases for annotations described in PEP 3107 (database mapping, | ||
foreign languages bridge) are not currently realistic given the | ||
prevalence of type annotations. |
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.
This is a bit misleading, since it looks like the main motivation is enabling non-typing related uses, but in fact enabling advanced typing features supported only by some type checkers is one of the motivating use cases. I think it is worth mentioning here.
pep-0592.rst
Outdated
``no_type_check`` functionality that current exists in the ``typing`` | ||
module which completely disables typechecking annotations on a function | ||
or a class, the ``Annotated`` type allows for both static typechecking | ||
of ``T`` (e.g., via MyPy or Pyre, which can safely ignore ``x``) |
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.
Please use normal-word capitalization for mypy, here and everywhere below: i.e. Mypy
at beginning of sentence, mypy
everywhere else.
pep-0592.rst
Outdated
~~~~~~~~~~~~~~~~~~~ | ||
|
||
Reading binary data | ||
^^^^^^^^^^^^^^^^^^^ |
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.
This is minor, but I generally don't like more than two nesting levels for sections. I would propose to make Motivating examples
a top-level section.
pep-0592.rst
Outdated
name: Annotated[str, struct.ctype("<10s")] | ||
serialnum: UnsignedShort | ||
school: SignedChar | ||
gradelevel: SignedChar |
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.
Please use 4 spaces indentation.
pep-0592.rst
Outdated
@struct.packedclass Student(NamedTuple): | ||
name: Annotated[str, struct.ctype("<10s")] | ||
|
||
:: |
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.
Why do you have this?
pep-0592.rst
Outdated
Class C: | ||
def const_method(self: Const[T]) -> int: | ||
... | ||
|
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 would add a short section about rejected ideas (even if there is currently just few of them, since more can appear during the discussion).
pep-0592.rst
Outdated
|
||
Writing ``typing.Annotated`` everywhere can be quite verbose; | ||
fortunately, the ability to alias annotations means that in practice we | ||
don’t expect clients to have to write lots of boilerplate code:: |
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.
don’t expect clients to have to write lots of boilerplate code:: | |
don't expect clients to have to write lots of boilerplate code:: |
pep-0592.rst
Outdated
Const = Annotated[T, my_annotations.CONST] | ||
|
||
Class C: | ||
def const_method(self: Const[T]) -> int: |
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.
def const_method(self: Const[T]) -> int: | |
def const_method(self: Const[List[int]]) -> int: |
pep-0592.rst
Outdated
|
||
# 'unpack' only uses the metadata within the type annotations | ||
Student.unpack(record) | ||
# Student(name=b'raymond ', serialnum=4658, school=264, gradelevel=8) |
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.
This example (and to certain extent the second example) is unnecessarily detailed. You focus too much on advocating for specific use case, instead of explaining the big picture with the example.
I’ll leave the review up to Ivan. |
@ilevkivskyi thanks for the suggestions. I tried to address everything you pointed out. Let me know if this version is better. |
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.
Thanks for updates! This is almost ready to be merged, I have few more comments. Also please don't rebase and/or force push, because it is harder to follow the reviews and GitHub doesn't send notifications when you do this. Just add new commits and merge when necessary.
pep-0592.rst
Outdated
@@ -0,0 +1,295 @@ | |||
PEP: 592 | |||
Title: External annotations in the typing module |
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.
Now I am thinking about a possible better title for this PEP. What do you think about "Flexible function and variable annotations"?
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.
"Mixing typing and non-typing annotations"? "Extra [meta]data in type hints"? "Non-typing information in type hints?" Just spitballing here.
pep-0592.rst
Outdated
analysis or at runtime. If a library (or tool) encounters a typehint | ||
``Annotated[T, x]`` and has no special logic for metadata ``x``, it | ||
should ignore it and simply treat the type as ``T``. Unlike the | ||
``no_type_check`` functionality that current exists in the ``typing`` |
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.
``no_type_check`` functionality that current exists in the ``typing`` | |
``no_type_check`` functionality that currently exists in the ``typing`` |
pep-0592.rst
Outdated
runtime (e.g.: dataclasses); having the ability to extend the typing annotations | ||
with external data would be a great boon for those libraries. | ||
|
||
Example:: |
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.
Make it clear that this example is about hypothetic functionality in cstruct
module.
pep-0592.rst
Outdated
name: Annotated[str, cstruct.ctype("<10s")] | ||
serialnum: UnsignedShort | ||
school: SignedChar | ||
gradelevel: SignedChar |
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.
Maybe we can keep only one SignedChar
field for brevity?
pep-0592.rst
Outdated
typing module and change mypy, PyCharm [pycharm]_, Pyre, | ||
pytype [pytype]_, etc... | ||
This is particularly important when working on open-source code that | ||
makes use of our new types, seeing as the code would not be immediately |
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.
makes use of our new types, seeing as the code would not be immediately | |
makes use of our these types, seeing as the code would not be immediately |
pep-0592.rst
Outdated
|
||
This is a somewhat cumbersome syntax but it allows us to iterate on this | ||
proof-of-concept and have people with type checkers (or other tools) that don't | ||
yet support this feature work in a codebase with tagged unions. We could easily |
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.
You often use "we" while it is not always totally clear whom do you mean.
pep-0592.rst
Outdated
int, ValueRange(3, 10), ValueRange(3, 10) | ||
] | ||
|
||
* ``Annotated`` can be used as a higher order aliases:: |
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.
* ``Annotated`` can be used as a higher order aliases:: | |
* ``Annotated`` can be used with nested and generic aliases:: |
pep-0592.rst
Outdated
and treat annotated type as the underlying type. For example, if we were | ||
to add an annotation that is not an instance of ``new_struct.ctype`` to the | ||
annotation for name (e.g., | ||
``Annotated[str, 'foo', new_struct.ctype("<10s")]``), the unpack method |
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.
Use same name for the hypothetic module here and in the first motivating example.
pep-0592.rst
Outdated
out of the returned value. Otherwise, the annotations will be returned | ||
unchanged:: | ||
|
||
@struct.packedclass Student(NamedTuple): |
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.
Something is still wrong with the formatting here, this is a syntax error.
pep-0592.rst
Outdated
Ideally, we should be able to introduce new types in a manner that allows for | ||
graceful degradation when clients do not have a custom mypy plugin | ||
[mypy-plugin]_, which would lower the barrier to development and ensure some | ||
degree of backward compatibility. |
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.
TBH, this sentence is still not very clear. In particular, it is not clear what is special about mypy here. Maybe add "for example" somewhere?
There's a conflict with another PEP introduced in #1032 now. |
Conflict in the PEP number (592), it should be added. |
@till-varoquaux You can grab the next number 593, also please let me know when this is ready for another review. |
@ilevkivskyi sorry for the incredible amount of procrastinating I've been doing. I owe you a box of chocolates for your patience... |
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.
OK, I think this is ready.
@gvanrossum Should I just merge this, or the PEP number should be officially assigned by a PEP-editor?
As a core dev you can merge it yourself. |
@till-varoquaux Please start a discussion on |
Btw @gvanrossum is there a chance this can get into Python 3.8? Technically there is still almost two weeks before beta1 :-) |
Not very likely then. That hardly sounds like enough to even have the PEP properly discussed on typing-sig. I haven't read it yet. How much code needs to be added to typing.py? Probably better to aim at typing_extensions. |
I think around hundred lines. But I can see how this is very tight.
OK, makes sense. We can add it there first and then move to |
If this lands in |
No description provided.