Python3 annotations by bluebird75 · Pull Request #74 · dropbox/pyannotate (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits33 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Implementation for #4 is done.
All tests are ported and pass successfully.
A few remarks on the implementation :
- command-line options are the same as mypy : -2, --py2, -3, --py3, --python-version
- the code will not annotate functions with an existing py2 or py3 annotation
- I ignored the concept of long form for python 3 annotations. Function with many arguments are still annotated inline.
- test_annotate.py and test_annotate_json.py have been renamed with a _py2 and _py3 suffix depending on what they are testing.
Looking forward for your feedback.
… return values annotations are supported so far.
…ut default values.
No support for * or ** arguments
…, -3, -py3, --python-version
Thanks! Reviewing this will take a bit of time, but your work is very much appreciated already!
Sure. I'll be patient then...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my initial review. It seems to work great!
| parser.add_argument('--py2', '-2', action='store_true', |
|---|
| help='''Annotate for Python 2 with comments (default)''') |
| parser.add_argument('--py3', '-3', action='store_true', |
| help='''Annotate for Python 3 with argument and return value annotations''') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably you could implement the latter two as action='store_const', dest='python_version', const='2' (or '3').
| sys.exit('--python-version must be 2 or 3') |
|---|
| if (args.py2 and args.py3) or (args.py2 and args.python_version) or (args.py3 and args.python_version): |
| sys.exit('You can not use multiple annotation version specifier') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you implement my suggestion above I would skip this check -- I would just let the last flag win (there are scenarios where that's actually useful).
| @@ -86,6 +93,16 @@ def main(args_override=None): |
|---|
| except OSError as err: |
| sys.exit("Can't open type info file: %s" % err) |
| if args.python_version not in (None, '2','3'): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a space after the second comma. (And if you add a default you won't have to check for None.)
| @@ -34,6 +34,13 @@ |
|---|
| help="Files and directories to update with annotations") |
| parser.add_argument('-s', '--only-simple', action='store_true', |
| help="Only annotate functions with trivial types") |
| parser.add_argument('--python-version', action='store', |
| help='''Choose annotation style, 2 for Python 2 with comments (the |
| default), 3 for Python 3 with direct annotation''' ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add default='2'. Then you won't have to check for None.
| into |
|---|
| into: |
| - with options {'annotation_style'='py2'} : |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use a comment as part of the code here (for a second or two I thought you were showing a with statement here).
| # Also add 'from typing import Any' at the top if needed. |
|---|
| self.patch_imports(argtypes + [restype], node) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should only be a single blank line between two methods.
| # Insert '# type: {annot}' comment. |
| # For reference, see lib2to3/fixes/fix_tuple_params.py in stdlib. |
| if len(children) >= 1 and children[0].type != token.NEWLINE: |
| # one liner function |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment feels misindented.
| it = iter([]) |
|---|
| elif len(args.children) == 0: |
| # function with 1 argument |
| it = iter( [ args ] ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't follow PEP 8 -- it should be iter([args]).
| rpar.changed() |
| def add_py2_annot( self, argtypes, restype, node, results): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No space after the (.
@bluebird75 I realize I took my time reviewing this. I am going on vacation for the next 3 weeks so unless I get terribly bored at my holiday destination I won't be reviewing your next commit very soon. Sorry! It's good work, it just needs one more round.
So, to sum-up, a few PEP8 issues and better use of argparse. That's not too bad. I was never very PEP8 friendly so that's not so surprising. :-) I'll work on all that in the coming weeks and provide an update.
You probably don't know but we've met before: I interviewed you at FOSDEM 2002 in Brussel when you came to receive the FSF award: http://www.freehackers.org/Fosdem_2002:_Guido_van_Rossum_interview . I can't believe it's been 16 years !
Here is a new version. Note that the CI failure is not due to the changes here but to some changes at Travis CI. See #76 for fixing them.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost ready to merge! Thanks again for your patience.
| @@ -34,6 +34,13 @@ |
|---|
| help="Files and directories to update with annotations") |
| parser.add_argument('-s', '--only-simple', action='store_true', |
| help="Only annotate functions with trivial types") |
| parser.add_argument('--python-version', action='store', default='2', |
| help='''Choose annotation style, 2 for Python 2 with comments (the |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The '''...''' thing seems cute but looks weird (I've never seen it before). While it seemingly works, I prefer using "..." for the help strings, and if they don't fit on the line, just use another "..." string on the next line (but make sure there's a space at the end of one or at the beginning of the other). The ones below should definitely switch back to "...".
| into |
|---|
| into a type annoted version: |
| def foo(self, bar, baz=12): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces around the = -- that's what the code actually does, and that's what PEP 8 requires. Ditto below.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest remarks are resolved now.
And there was great rejoicing!
Seriously, thanks for hanging in there and shaving some yaks. We should celebrate by doing a point release!
I'll be more than happy to celebrate with a release. I'll see where I can contribute next. Multi-source type annotations looks like a big topic to me.
Do we have a release plan for this?
carver added a commit to carver/pyannotate that referenced this pull request
gvanrossum pushed a commit that referenced this pull request
as of v1.0.7, merged in #74