-
Notifications
You must be signed in to change notification settings - Fork 3k
Python: Add tox commands for code format and type checking #3282 #3423
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
Conversation
|
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
| 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
samredai
left a comment
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
| bandit | ||
| 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! |
Uh oh!
There was an error while loading. Please reload this page.