-
Notifications
You must be signed in to change notification settings - Fork 3k
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 option to silence warnings related to deprecation of Python versions #6739
Conversation
@pradyunsg All tests have passed. I think this is ready to be reviewed. |
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.
Hi @victorvpaulo, sorry it has taken so long to get feedback to you.
I have a few minor comments, and this will also need tests. I would create 3:
- Test that warning shows up as expected - this should only run on CPython + Python 2
- Test that warning does not show up anywhere else - this should run on not (CPython + Python 2)
- Test that warning does not show up on CPython + Python 2 when the flag is passed.
For examples of checking pip output, please look at test_installing_scripts_outside_path_can_suppress_warning
in tests/functional/test_install.py
. We probably do not want to do a pip install
since that can add some time to the overall test execution, but something like a pip debug
should still cause the warning to print. I didn't see an existing test for this functionality submitted with the original change (#6147), so it can probably go in tests/functional/test_warning.py
.
Not sure why GitHub is showing that all of these commits would be added. Can you rebase so your commit is the only one on top of the current master branch? |
I'm sorry, i think i used merge instead of rebase to update my feature branch. |
If suggestions are still possible, I’d wish the option name is more consistent with others. Some potential names I can think of are
The latter two have a slightly different sematic`, but I feel the current warning (and its implementation) is kind of like the pip version check. |
Hi, thank you for your suggestion. |
…correctly and can be silenced by a flag.
@chrahunt I created the tests you specified and made the other necessary changes. I believe this is ready for review. |
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.
This looks good! I have a few minor comments. Can we also add one test passing --no-python-version-warning
on not Python 2? We shouldn't need to actually check anything from the result for that one since the script
helper already asserts the return code is zero and no stderr is output.
… test/lib/__init__.py and use them in test_warning.py
…hing if python version is not 2
@chrahunt I think the latest commits address your points. I did some tests locally to ensure that the move of the skip decorators declarations to |
Thank you! Yes, I think it does. |
Thanks again @victorvpaulo! |
Thank you for your reviews @chrahunt. The guidance you provided is really important to a newbie oss contributor like me. |
Implements #6673, Helps with #6231.
@pradyunsg