Skip to content
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

Added support for less verbose version information #7169

Merged
merged 6 commits into from
May 23, 2020

Conversation

debugduck
Copy link
Contributor

@debugduck debugduck commented May 5, 2020

Fixes #7128

@debugduck debugduck changed the title Added support for less verbose version information. Added support for less verbose version information May 6, 2020
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a 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

@hugovk
Copy link
Member

hugovk commented May 6, 2020

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

Copy link
Member

@nicoddemus nicoddemus left a 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.

@hugovk
Copy link
Member

hugovk commented May 6, 2020

This is in line on how other tools work, such as black, pre-commit and Python itself.

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
...

@nicoddemus
Copy link
Member

nicoddemus commented May 6, 2020

Only Python itself:

Oh I misunderstood this comment then: #3692 (comment)

Thanks, I will update my suggestion.

@asottile
Copy link
Member

@debugduck hi! still working on this?

@debugduck
Copy link
Contributor Author

@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.

@nicoddemus
Copy link
Member

@debugduck thanks for the heads up!

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.

Feel free to push what you have so we can take a look, we can probably help out with it. 👍

@debugduck
Copy link
Contributor Author

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.

assert result.ret == ExitCode.USAGE_ERROR


def test_help_and_version_verbose_after_argument_error(testdir):
Copy link
Member

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:

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!

Copy link
Contributor Author

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! :)

@@ -1200,52 +1200,6 @@ def test_help_via_addopts(testdir):
)


def test_help_and_version_after_argument_error(testdir):
Copy link
Member

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.

Copy link
Member

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Will do.

Copy link
Contributor Author

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.

Copy link
Member

@nicoddemus nicoddemus left a 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)

@debugduck
Copy link
Contributor Author

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.

@debugduck
Copy link
Contributor Author

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

I think these should be resolved now--I set a default of 0 and also reset my metadata.

@nicoddemus nicoddemus merged commit 79701c6 into pytest-dev:master May 23, 2020
@nicoddemus
Copy link
Member

Thanks again @debugduck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest --version less verbose by default
5 participants