-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Added support for less verbose version information #7169
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
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 like a good start, well done
it seems like a few additional tests are affected, i'm not sure why None creeps up there.
i believe it should be fine to handle it with a "is None" or putting in a default of 0 (untested)
it seems like your git metadata on the commit is not connected to your github profile, you might want to reset the author metadata to something thats connected
You can also add new ones to be linked at https://github.com/settings/emails |
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 @debugduck!
Please also add yourself to AUTHORS
, and include a new changelog entry 7128.improvement.rst
:
`pytest --version` now displays just the pytest version, while `pytest --version --version` displays more verbose information including plugins.
Only Python itself: $ pre-commit --version
pre-commit 2.3.0
$ pre-commit --version --version
pre-commit 2.3.0 $ black --version
black, version 19.10b0
$ black --version --version
black, version 19.10b0 $ python --version
Python 3.8.2
$ python --version --version
Python 3.8.2 (v3.8.2:7b3ab5921f, Feb 24 2020, 17:52:18)
[Clang 6.0 (clang-600.0.57)] $ python --help
...
-V : print the Python version number and exit (also --version)
when given twice, print more information about the build
... |
Oh I misunderstood this comment then: #3692 (comment) Thanks, I will update my suggestion. |
@debugduck hi! still working on this? |
hi there--I'm still working on this. Apologies, as I've been moving and work has been kind of busy. But I will get on this as soon as I can. I'm having a bit of trouble with one of the tests for test_config.py and trying to figure out why my change broke it. |
@debugduck thanks for the heads up!
Feel free to push what you have so we can take a look, we can probably help out with it. 👍 |
So I added a test to test_config.py and I'm not really sure why test_help_and_version_verbose_after_argument_error is failing but the test in test_helpconfig.py which tests the verbose version argument works. I tried to take a look at it but I think there might be something that I'm just missing. I'm not even completely sure that it's appropriate to test the verbose version argument here, but just figured it was since we were testing the regular version argument as well. |
testing/test_config.py
Outdated
assert result.ret == ExitCode.USAGE_ERROR | ||
|
||
|
||
def test_help_and_version_verbose_after_argument_error(testdir): |
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 @debugduck!
This is not producing the output because there's special handling when there's an usage error:
pytest/src/_pytest/config/__init__.py
Lines 871 to 893 in 5a6296a
def pytest_cmdline_parse(self, pluginmanager, args): | |
try: | |
self.parse(args) | |
except UsageError: | |
# Handle --version and --help here in a minimal fashion. | |
# This gets done via helpconfig normally, but its | |
# pytest_cmdline_main is not called in case of errors. | |
if getattr(self.option, "version", False) or "--version" in args: | |
from _pytest.helpconfig import showversion | |
showversion(self) | |
elif ( | |
getattr(self.option, "help", False) or "--help" in args or "-h" in args | |
): | |
self._parser._getparser().print_help() | |
sys.stdout.write( | |
"\nNOTE: displaying only minimal help due to UsageError.\n\n" | |
) | |
raise | |
return self |
When that happens, it is best to just stick to showing the minimal version information. 👍
So while your instincts were correct in adding this test, I think we can just remove it.
Thanks!
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.
Gotcha!
Okay, I removed it. I believe I resolved the git metadata and I also added the documentation for the change as well.
Thanks! :)
testing/test_config.py
Outdated
@@ -1200,52 +1200,6 @@ def test_help_via_addopts(testdir): | |||
) | |||
|
|||
|
|||
def test_help_and_version_after_argument_error(testdir): |
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.
Oh sorry I don't think I was clear: this test you should keep, I meant to remove the test you have added before (test_help_and_version_verbose_after_argument_error
).
This test (and the adjustment you made to it) is necessary: when we get a parser error, we need to ensure we still display at least the pytest version.
I asked you to remove the "verbose" kind of this same test because the output should actually be the same as the "non-verbose" kind: "--verbose --verbose" output normally includes plugins, which are not loaded when a parser error happens anyway, so we regardless fall back to the "non-verbose version" information.
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.
IOW: please revert your dfcec18 commit and just delete test_help_and_version_verbose_after_argument_error
, leaving test_help_and_version_after_argument_error
as is. 👍
Thanks!
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.
Gotcha! Will do.
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.
Hey there--so I checked this today and realized that I accidentally deleted the original test test_help_and_version_after_argument_error
while trying to delete onlytest_help_and_version_verbose_after_argument_error
.
I'm really sorry for that confusing mistake, but I've pushed another commit to correct it.
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 a lot @debugduck, we appreciate it!
(The py37 failure on Windows can be ignored, it is a flaky test which will be handled separately)
Thank you so much for the opportunity to contribute! You've been so supportive and I only hope I can contribute again at some point. |
I think these should be resolved now--I set a default of 0 and also reset my metadata. |
Thanks again @debugduck! |
Fixes #7128