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

fix behavior of show_default with context_settings #115

Closed
wants to merge 1 commit into from

Conversation

u-shiori
Copy link
Contributor

Closes #114

Core change

In _get_help_record function, added an argument ctx and added a variable show_default.

This is almost the same implementation as click's _get_help_record function

Copy link
Member

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

This is a good change. Thanks very much. Please see my comments inline. Nothing huge, hopefully, and I'll happily merge and release this once they're tackled.

@@ -43,7 +43,7 @@ def _get_usage(ctx: click.Context) -> str:
return formatter.getvalue().rstrip('\n') # type: ignore


def _get_help_record(opt: click.Option) -> ty.Tuple[str, str]:
def _get_help_record(opt: click.Option, ctx: click.Context) -> ty.Tuple[str, str]:
Copy link
Member

Choose a reason for hiding this comment

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

Can you pass ctx as the first argument instead, please. Ditto for the other modified functions.

if opt.show_default is not None:
show_default = opt.show_default
else:
show_default = ctx.show_default
Copy link
Member

Choose a reason for hiding this comment

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

This is available in click 7.x so we don't need version-specific logic 👍

However, the click 8.1.x docs state:

Changed in version 8.1: The show_default parameter is overridden by Command.show_default, instead of the other way around.

Should we replicate this behaviour here? It feels like we should? If we don't, we might want to add a note here that we intentionally decided not to.

@@ -231,6 +231,40 @@ def foobar(bar):
'\n'.join(output),
)

def test_show_default(self):
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the test. Could you also add a release note? For example:

pip install --user reno
reno new add-show_default-from-context-support

@stephenfin
Copy link
Member

Looks like you also need to run the linters. tox -e style should do the trick.

@stephenfin
Copy link
Member

No activity on this for some time. This is now being addressed in #131.

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.

Missing show_default detection via context_settings.
2 participants