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

Display empty or None help parameter for parser option as empty string #7427

Merged
merged 10 commits into from Jul 29, 2020
Merged

Display empty or None help parameter for parser option as empty string #7427

merged 10 commits into from Jul 29, 2020

Conversation

hp310780
Copy link
Contributor

In response to Bug #7394 , this PR updates pytest --help to parse an empty string or None as an empty string for the help parameter.

closes #7394

Copy link
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @hp310780! I left some comments

Also, don't forget to add a changelog entry

src/_pytest/helpconfig.py Outdated Show resolved Hide resolved
testing/test_helpconfig.py Outdated Show resolved Hide resolved
@gnikonorov gnikonorov self-requested a review June 28, 2020 14:58
Copy link
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Requesting changes as above, sorry I checked approve on accident

@gnikonorov
Copy link
Member

I was thinking something like [No description provided] so the end user knows the state of the option. But, happy to change to throwing an error (a TypeError like a missing function param?) on help being None or "", let me know what you would like.

Let’s get some more input. @RonnyPfannschmidt @nicoddemus do we think that in the event of no help provided we should throw an error, or supply a default help

@gnikonorov
Copy link
Member

@hp310780 - I personally vote in favor of an exception. Are you ok with going with that plan while we wait for a review

@hp310780
Copy link
Contributor Author

hp310780 commented Jul 2, 2020

@hp310780 - I personally vote in favor of an exception. Are you ok with going with that plan while we wait for a review

Yep! Changed. Let me know of any feedback. Thanks.

Copy link
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

I think other than the comment I left the change looks good

src/_pytest/helpconfig.py Outdated Show resolved Hide resolved
Copy link
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Thanks @hp310780, look good!

Let's wait for Ronny to take a look as well

@gnikonorov
Copy link
Member

bump @RonnyPfannschmidt @nicoddemus @bluetech - could someone else take a look at this PR?

src/_pytest/helpconfig.py Outdated Show resolved Hide resolved
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for the update @hp310780. I left a couple of comments.

changelog/7394.bugfix.rst Outdated Show resolved Hide resolved
testing/test_helpconfig.py Outdated Show resolved Hide resolved
@nicoddemus
Copy link
Member

LGTM, the requests/suggestions by @bluetech are on point! 👍

@hp310780 hp310780 requested a review from bluetech July 26, 2020 20:06
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Almost there, just one last question. I had hoped the improved test would answer it but it didn't.

src/_pytest/helpconfig.py Outdated Show resolved Hide resolved
@hp310780 hp310780 requested a review from bluetech July 28, 2020 18:24
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks @hp310780! This looks good now. I will merge it right after pytest 6.0.0 is released (I don't want to interfere with the ongoing release).

@bluetech bluetech merged commit 27a4c6c into pytest-dev:master Jul 29, 2020
@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Jul 29, 2020
nicoddemus pushed a commit to nicoddemus/pytest that referenced this pull request Jul 29, 2020
@nicoddemus
Copy link
Member

Backport: #7577

nicoddemus added a commit that referenced this pull request Jul 29, 2020
[6.0.x] Fix --help crash on add_ini(.., help='') and improve message on help=None (#7427)
@nicoddemus nicoddemus added backported PR has been backported to the current bug-fix branch and removed needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch labels Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported PR has been backported to the current bug-fix branch
Projects
None yet
4 participants