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

Hide default value when show_default is False #2509

Merged
merged 5 commits into from
Nov 2, 2024

Conversation

pfhayes
Copy link
Contributor

@pfhayes pfhayes commented May 5, 2023

Previously, when show_default was False on an Option, the value could still appear in the prompt. Remove this so that sensitive values can be hidden from prompts.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

Copy link
Collaborator

@saroad2 saroad2 left a comment

Choose a reason for hiding this comment

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

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.

@@ -2865,6 +2865,7 @@ def prompt_for_value(self, ctx: Context) -> t.Any:
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),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not simply: show_default=self.show_default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@@ -446,3 +446,13 @@ def cli(password):

if prompt == "Confirm Password":
assert "Confirm Password: " in result.output


def test_show_default_false_hides_prompt(runner):
Copy link
Collaborator

@saroad2 saroad2 May 7, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/test_termui.py Show resolved Hide resolved
Copy link
Collaborator

@saroad2 saroad2 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 minding my review.

I added two more small suggestions. After you respond to those, we could merge this PR.

@@ -2865,6 +2865,7 @@ def prompt_for_value(self, ctx: Context) -> t.Any:
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),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

CHANGES.rst Outdated Show resolved Hide resolved
@saroad2
Copy link
Collaborator

saroad2 commented May 8, 2023

Also, this PR doesn't pass pre-commit at the moment. You might want to run pre-commit run -a in order to make sure that all files are according to Click's style guideline.

@pfhayes
Copy link
Contributor Author

pfhayes commented May 8, 2023

Issues fixed. Added comment and also restructured code to make behavior of show_default arg to prompt more explicit

@davidism davidism added this to the 8.2.0 milestone Jul 4, 2023
@AndreasBackx AndreasBackx mentioned this pull request Oct 20, 2024
39 tasks
@AndreasBackx AndreasBackx merged commit a6e0d29 into pallets:main Nov 2, 2024
12 checks passed
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.

4 participants