-
-
Notifications
You must be signed in to change notification settings - Fork 676
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
add flake8-annotations package for type linting #1061
Conversation
- add pkg in requirements-dev.txt - require pkg in pre-commit config - add some easily resolved annotations in tests - ignore some type warnings in tox.ini
Codecov Report
@@ Coverage Diff @@
## master #1061 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 2223 2223
Branches 350 350
=========================================
Hits 2223 2223 Continue to review full report at Codecov.
|
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 the controbution @nthall. Looks good, but it seems like CI is failing on a number of Python versions and OSes. You may need to replace list[str]
with List[str]
(List
generic from the typings library).
@jadchaar thanks - sorry for that. I've pushed a fix. Frankly one reason I wanted to tackle this was to improve my familiarity with type annotations, as most of the code I get to work on right now is still on 2.7 with no plans for upgrade or mypy integration. |
Ah, this is a tougher one. Down in the dependency chain for |
Since we are only running |
OK. I've updated Happy Halloween! |
I think this is done now? The only thing arguably left would be to rebase it into a cleaner commit - if you want me to do that, I will. |
No need for the cleaner commit. We can do a simple squash on it. The PR looks good to me, but I'm just waiting on approval from @jadchaar or @krisfremen before we pull it in. |
Looks great thanks for the awesome work @nthall! |
Pull Request Checklist
Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:
tox
ormake test
to find out!).tox -e lint
ormake lint
to find out!).master
branch.If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!
Description of Changes
Closes: #856
This change adds the
flake8-annotations
plugin to enable linting of type annotations viapre-commit
/tox
. In order to enable that without breaking the build, I've added some annotation warnings to the flake8 ignore lists. I thought it would best be left to the maintainers to decide whether it would be more appropriate to address those warnings in separate commits/PRs.The ignored warnings, and some brief discussion:
ANN101 Missing type annotation for self in method
/ANN102 Missing type annotation for cls in classmethod
flake8-annotations
warns on this case anyway; given the large number of changes required to "fix" this warning I thought it better here to ignore them and have a smaller commit that still advanced the state of the project. Of course if they're desired it would be cleaner to address that issue separately from whether to lint for annotations at all.tests
dir:ANN201 Missing return type annotation for public function
ANN001 Missing type annotation for function argument
pytest
fixtures inconftest.py
; again, seems like a change worth discussing separately from existing annotations.