Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
the-knights-who-say-ni commented Apr 26, 2019
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! |
ilevkivskyi left a comment
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.
gvanrossum commented Apr 27, 2019
I’ll leave the review up to Ivan. |
till-varoquaux commented Apr 29, 2019
@ilevkivskyi thanks for the suggestions. I tried to address everything you pointed out. Let me know if this version is better. |
ilevkivskyi left a comment
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.
| 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?
jstasiak commented May 14, 2019
There's a conflict with another PEP introduced in #1032 now. |
pfalcon commented May 14, 2019
Conflict in the PEP number (592), it should be added. |
ilevkivskyi commented May 14, 2019
@till-varoquaux You can grab the next number 593, also please let me know when this is ready for another review. |
till-varoquaux commented May 16, 2019
@ilevkivskyi sorry for the incredible amount of procrastinating I've been doing. I owe you a box of chocolates for your patience... |
ilevkivskyi left a comment
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?
gvanrossum commented May 19, 2019
As a core dev you can merge it yourself. |
ilevkivskyi commented May 19, 2019
@till-varoquaux Please start a discussion on |
ilevkivskyi commented May 19, 2019
Btw @gvanrossum is there a chance this can get into Python 3.8? Technically there is still almost two weeks before beta1 :-) |
gvanrossum commented May 19, 2019
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. |
ilevkivskyi commented May 19, 2019
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 |
jstasiak commented May 20, 2019
If this lands in |
No description provided.