Hide default value when show_default is False#2509
Hide default value when show_default is False#2509AndreasBackx merged 5 commits intopallets:mainfrom
Conversation
saroad2
left a comment
saroad2
left a comment
There was a problem hiding this comment.
Hey, thanks for the contributaion!
This PR makes sense to me. I added some minor comments about the PR.
After fixing those, I could merge it.
src/click/core.py
Outdated
| show_choices=self.show_choices, | ||
| confirmation_prompt=self.confirmation_prompt, | ||
| value_proc=lambda x: self.process_value(ctx, x), | ||
| show_default=(False if self.show_default is False else True), |
There was a problem hiding this comment.
Why not simply: show_default=self.show_default?
There was a problem hiding this comment.
In this class, self.show_default is a Union[bool, str, None], but termui.prompt accepts only bool. Passing through show_default directly gives a type error, and also appears to do the wrong things for values of type filenam
There was a problem hiding this comment.
Got it. Thanks for the explanation.
Would you mind writing it down as a comment in the code? I think it would help others who read this line of code for the first time.
tests/test_termui.py
Outdated
| assert "Confirm Password: " in result.output | ||
|
|
||
|
|
||
| def test_show_default_false_hides_prompt(runner): |
There was a problem hiding this comment.
This test name can be clearer in order to explain what does this test check.
Mybe something like "test_false_show_default_cause_no_default_display_in_prompt".
saroad2
left a comment
saroad2
left a comment
There was a problem hiding this comment.
Thanks for minding my review.
I added two more small suggestions. After you respond to those, we could merge this PR.
src/click/core.py
Outdated
| show_choices=self.show_choices, | ||
| confirmation_prompt=self.confirmation_prompt, | ||
| value_proc=lambda x: self.process_value(ctx, x), | ||
| show_default=(False if self.show_default is False else True), |
There was a problem hiding this comment.
Got it. Thanks for the explanation.
Would you mind writing it down as a comment in the code? I think it would help others who read this line of code for the first time.
|
Also, this PR doesn't pass pre-commit at the moment. You might want to run |
|
Issues fixed. Added comment and also restructured code to make behavior of |
AndreasBackx
force-pushed
the
hide-default-when-specified
branch
from
fb3c456 to
3fb7d3d
Compare
4 participants
Previously, when
show_defaultwas False on anOption, the value could still appear in the prompt. Remove this so that sensitive values can be hidden from prompts.Checklist:
CHANGES.rstsummarizing the change and linking to the issue... versionchanged::entries in any relevant code docs.pre-commithooks and fix any issues.pytestandtox, no tests failed.