GeezerVison is showing github.com — Implement PEP 526 · Issue #72172 · python/cpython
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2016-09-09.03:54:03.020>
created_at = <Date 2016-09-06.23:48:46.989>
labels = ['interpreter-core', 'type-feature']
title = 'Implement PEP 526'
updated_at = <Date 2016-09-09.21:48:16.681>
user = 'https://github.com/gvanrossum' bugs.python.org fields:
activity = <Date 2016-09-09.21:48:16.681>
actor = 'python-dev'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2016-09-09.03:54:03.020>
closer = 'yselivanov'
components = ['Interpreter Core']
creation = <Date 2016-09-06.23:48:46.989>
creator = 'gvanrossum'
dependencies = []
files = ['44415', '44416', '44469', '44470', '44474', '44478']
hgrepos = []
issue_num = 27985
keywords = ['patch']
message_count = 33.0
messages = ['274672', '274693', '274695', '274914', '274918', '274919', '274925', '274929', '274930', '274938', '274954', '275079', '275080', '275081', '275083', '275085', '275088', '275097', '275115', '275117', '275136', '275158', '275163', '275169', '275184', '275191', '275192', '275245', '275246', '275247', '275265', '275267', '275458']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'python-dev', 'yselivanov', 'levkivskyi']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27985'
versions = ['Python 3.6'] Activitygvanrossum commented on Sep 6, 2016 Pending PEP-526's acceptance, I am inviting Ivan Levkivskyi to upload his patch here so it can be reviewed. ilevkivskyi commented on Sep 7, 2016 Here is the patch for PEP-526 implementation ilevkivskyi commented on Sep 7, 2016 Oops, sorry, forgot to add new files when converting from git to hg, here is the full patch. gvanrossum commented on Sep 7, 2016 Hey Ivan, Brett and I divided the review, he started at the bottom and I started at the top, we're meeting at Lib/typing.py. gvanrossum commented on Sep 7, 2016 I played with the REPL, and found this:
>>> del __annotations__
del __annotations__
>>> x: int = 0
x: int = 0
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
NameError: __annotations__ not found
>>> I would expect this to re-create __annotations__. ilevkivskyi commented on Sep 7, 2016 We discussed this at some point on python/typing. At that time we decided that
del __annotations__
x: int
Should be an error. Do you think that it should behave in a different way in interactive REPL and/or at module level? ilevkivskyi commented on Sep 8, 2016 I could change STORE_ANNOTATION opcode so that it will recreate __annotations__ if __name__ == '__main__'.
Or do you now think that it should re-create it always? I still think that always re-creating __annotations__ if they don't exist seems like silencing a possible error. As I mentioned in previous discussion, I think we should allow people to explicitly del __annotations__ (for example if someone wants to make a class with annotations that are "invisible" to runtime tools) and warn them if later they use annotations. gvanrossum commented on Sep 8, 2016 Each statement at the REPL should re-initialize __annotations__ if it
contains any annotations. I think this is how exec() already works. It
adds __annotations__ to the namespace as needed, but just updates it
if present. Inside a class it's different, that should be considered a
single block. ilevkivskyi commented on Sep 8, 2016
I think this is how exec() already works
This is not exactly like this, exec() emits ast.Module while interactive input emits ast.Interactive (this one skips compiler_body in compile.c), but I think I have got your point, will fix this. ilevkivskyi commented on Sep 8, 2016 Guido,
I fixed __annotations__ in interactive REPL, will fix other minor things and submit a new patch tomorrow morning.
There are two important questions in typing.py:
Why is the second isinstance() needed? Isn't _ClassVar a subclass of
TypingMeta
and
This being a metaclass, the name should end in Meta, like most
other subclasses of TypingMeta. (And they don't have a leading _,
though they're still not public.)
The problem with ClassVar is that it is not actually a class, and _ClassVar is not a metaclass, it is just a class. I deviated from the common pattern in module for the following reason. ClassVar is going to be extensively used at class scope, where all annotations are evaluated. Therefore I wanted not to create new class objects by __getitem__ like it is going for other classes like Union etc (I tried to timeit this and ClassVar is almost ten times faster than Union)
What do you think?
Should I keep it like this, or rewrite in a common pattern for the module? gvanrossum commented on Sep 8, 2016 Oh, dang, I misread the definition of _ClassVar. It's fine then!
Looking forward to the next installment. ilevkivskyi commented on Sep 8, 2016 Here is the new patch. I hope I didn't miss any comment and fixed everything.
There is still a refleak to fix. gvanrossum commented on Sep 8, 2016 Ivan, I have no idea how to integrate your patch and Yury's. I only use Mercurial here, I don't trust the github clone (how far behind is it?). Please sort this out yourself. I am also going through the review on rietveld again. ilevkivskyi commented on Sep 8, 2016 Guido,
I resolved merge conflicts in patch v4 (if it will complain, this could be because of graminit.c or importlib_external.h, just ignore those, they will be regenerated during build).
Please take a look and sorry for a delay. 1st1 commented on Sep 8, 2016
Please take a look and sorry for a delay.
Ivan, I'll be the one merging the patch. Will be looking over it soon. I might fix some nits myself, so if it applies cleanly to the default branch and all tests pass - then I'll handle the rest.
Please also run the tests with -R3:3 (grammar/parser tests specifically) ilevkivskyi commented on Sep 8, 2016 Thank you Yury,
I usually do
./python -m test -R : test___all__ test_dis test_grammar test_opcodes test_parser test_pydoc test_symtable test_tools test_typing
and then
./python -m test -j3 -u all
I just run tests with -R3:3 it also works. gvanrossum commented on Sep 8, 2016 (I've renamed the patches so they line up with the numbering in the code review tool.) 1st1 commented on Sep 8, 2016 Ivan, is "hg-pep-526-v5.diff" patch the one I can commit? ilevkivskyi commented on Sep 8, 2016 Yes,
This is the latest patch that I tested and with resolved merge conflicts. python-dev commented on Sep 9, 2016 1st1 commented on Sep 9, 2016 Committed. Congrats Ivan, it's a very serious contribution. Thank you. gvanrossum commented on Sep 9, 2016 W00t! Thank Ivan for the code! And thanks Yury and Brett for the review. ilevkivskyi commented on Sep 9, 2016 Thank you Guido, Yury, Brett, and Serhiy!
It is first time I make such kind of contribution. It was a great experience and a great opportunity to better understand CPython internals (I already have several ideas on what to work next :-)
I will soon submit a PR to python/typing with copy of typing changes (also Python2 version) and a patch to remove com2ann script to a separate repo as discussed with Guido. 1st1 commented on Sep 9, 2016 You're welcome.
It is first time I make such kind of contribution. It was a great experience and a great opportunity to better understand CPython internals (I already have several ideas on what to work next :-)
If you need any help/mentoring please don't hesitate to approach me directly. python-dev commented on Sep 9, 2016
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|