-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Python: Add tox commands for code format and type checking #3282 #3423
Conversation
cabhishek
commented
Oct 29, 2021
•
edited
Loading
edited
- This PR adds tox commands to automate code formatting and type checking.
- tox uses following
- Core tox commands
- Future enhancements
- We'll need a follow PR to run these commands over the codebase.
One open question: Should we pin |
@cabhishek I think pinning makes sense! |
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.
Yep, I think we should have a requirements file for pinning.
python/setup.cfg
Outdated
@@ -33,6 +33,7 @@ license_files = | |||
classifiers = | |||
License :: OSI Approved :: Apache Software License | |||
Operating System :: OS Independent | |||
Programming Language :: Python :: 3.6 |
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.
QQ, wondering if we should support 3.6? Per recent sync meeting, the plan is to support just py37,py38,py39
or higher versions.
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 vote that we don't support 3.6 unless someone really feels we should. Looking at endoflife.date/python shows that python 3.6 security support ends in a little over a month.
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 think that we discussed this in one of the python syncs and we agreed that supporting 3.6 was a bad idea.
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.
Reverted changes and dropped 3.6 support.
a96da6e
to
39ff690
Compare
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.
One small nit comment that I don't think needs to block merging this PR. LGTM! Thanks @cabhishek
python/tox.ini
Outdated
commands = | ||
bandit --ini tox.ini -r src | ||
mypy src |
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.
nit: Should we add --no-implicit-optional
here? This will require the explicit x: Optional[int] = None
and will fail on x: int = None
. The latter always reads odd to me, "must be an int, but by default it's None". The Optional type hint feels clearer.
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.
Added the flag. And given how common it is to have code like x: int = None
I guess the future version of mypy will make that flag the default.
39ff690
to
ab95b58
Compare
Thanks, @cabhishek! |