Skip to content
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

Parse mypy type comments #220

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Conversation

jingw
Copy link

@jingw jingw commented Aug 8, 2020

Description

This prevents type-only imports from being flagged as unused.

Implementation decision note: As typed_ast will not be updated to support parsing Python 3.8+ syntax, I'm choosing not to use it. The downside of using Python's built-in type comment support is that it requires Python 3.8+. Someone more motivated than me could potentially make vulture use both, but I don't feel it's worth the complexity/effort given typed_ast seems likely to eventually die.

Related Issue

Fixes #152
See also #153

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation in the README.md file. (but do you want this in the README? not sure how much you want there versus left to --help)
  • I have added an entry in CHANGELOG.md.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to remove the command line option and simply parse the type comments if the ast module of the Python interpreter supports them. I'll do a detailed review once this is changed.

@jingw
Copy link
Author

jingw commented Aug 9, 2020

Now that I look around, I see flake8 complains about bad type comments by default, without any obvious option to disable that. I'll go ahead and make the proposed change.

Originally, I decided to mirror the logic in https://bugs.python.org/issue35766 (the PR that added type_comments to ast.parse)

By default, ast.parse() should not return type comments, since this would reject some perfectly good Python code (with sonething looking like a type comment in a place where the grammar doesn’t allow it). But passing an new flag will cause the tokenizer to process type comments and the returned tree will contain them.

That said, I imagine vulture does not need to be as careful as the standard library with regards to backwards compatibility.

@jingw
Copy link
Author

jingw commented Aug 9, 2020

Done! I've made it automatically gate on Python 3.8+

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I would have expected that we need much more code for this feature :-) I only have some small nitpicks.

vulture/core.py Outdated Show resolved Hide resolved
vulture/core.py Outdated Show resolved Hide resolved
tests/test_scavenging.py Show resolved Hide resolved
@jingw jingw changed the title Add option to parse type comments Parse mypy type comments Aug 9, 2020
This prevents type-only imports from being flagged as unused.

Implementation decision note: As typed_ast will not be updated to support parsing Python 3.8+ syntax, I'm choosing not to use it. The downside of using Python's built-in type comment support is that it requires Python 3.8+. Someone more motivated than me could potentially make vulture use both, but I don't feel it's worth the complexity/effort given typed_ast seems likely to eventually die.

Fixes jendrikseipp#152
See also jendrikseipp#153
@jendrikseipp jendrikseipp merged commit 4c9fb34 into jendrikseipp:master Aug 10, 2020
@jendrikseipp
Copy link
Owner

Thanks a lot for your work on this!

@jingw jingw deleted the type-comments branch August 14, 2020 03:15
@djh82
Copy link

djh82 commented Feb 3, 2021

Apologies for being the PR necromancer; what was the rationale behind only enabling this for py3.8+? Especially given 3.6+ supports typing (and was introduced in the now EOL 3.5)? I didnt want to raise a new issue, if there was a good reason for it. Thanks!

@jendrikseipp
Copy link
Owner

Please see the issue description for the reasoning. In short: we don't think that the extra work is worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore type checking imports
3 participants