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: Show message when showHelpOnFail is chained globally #2154

Merged
merged 4 commits into from Mar 31, 2022
Merged

fix: Show message when showHelpOnFail is chained globally #2154

merged 4 commits into from Mar 31, 2022

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Mar 26, 2022

Addresses: #2085

Problem

When showHelpOnFail is used globally, it doesn't work.
This is because usage.reset() sets failMessage to null.

Solution

I added a globalFailMessage variable to capture the global mesage.
On fail, failMessage || globalFailMessage is printed. I figured if both are provided, the more specific message should be shown.

Other

I saw that null | undefined was used a lot, so I made type nil. (Idea borrowed from lodash isNil)

lib/usage.ts Outdated

// If global context, set globalFailMessage
// Addresses: https://github.com/yargs/yargs/issues/2085
if (yargs.getInternalMethods().getContext().commands.length === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an easier way to figure out that this is in the global context? (I can move this logic to a helper function and call it here)

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of pulling this into a private helper method, then if we can think of a better way to detect we're in the global context we can swap out the logic.

@jly36963 jly36963 requested a review from bcoe March 26, 2022 15:35
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

One suggested refactor.

lib/usage.ts Outdated

// If global context, set globalFailMessage
// Addresses: https://github.com/yargs/yargs/issues/2085
if (yargs.getInternalMethods().getContext().commands.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of pulling this into a private helper method, then if we can think of a better way to detect we're in the global context we can swap out the logic.

@bcoe
Copy link
Member

bcoe commented Mar 28, 2022

LGTM, with a suggested refactor. I would be surprised if we're not already detecting the global context, vs., a command handler context somewhere else in the codebase. Perhaps look in command.ts (nothing is immediately coming to my mind).

@jly36963
Copy link
Contributor Author

I realized that checking commands.length === 0 yields a false positive when running the default command.
Instead, I set a new property this.#isGlobalContext to true, and when the runCommand logic begins I set it to false.

@jly36963 jly36963 requested a review from bcoe March 30, 2022 22:34
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

As always, thank you for the contributions.

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.

None yet

2 participants