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

Make --help and --version work with allowUnknownFlags: false #198

Closed
wants to merge 6 commits into from

Conversation

msabramo
Copy link

@msabramo msabramo commented Sep 15, 2021

Make --help and --version work with allowUnknownFlags: false

Fixes #197

Issue originally discovered with chalk-cli in chalk/chalk-cli#32

Make `--help` and `--version` work with `allowUnknownFlags: false`

Fixes: sindresorhusGH-197
@sindresorhus
Copy link
Owner

Can you add tests? 🙏

@sindresorhus sindresorhus changed the title Make --help/--version work w/ !allowUnknownFlags Make --help and --version work with allowUnknownFlags: false Sep 15, 2021
@msabramo
Copy link
Author

Yeah I thought of that right after I submitted this. 😄

@msabramo
Copy link
Author

See 368e760 for tests.

Unfortunately, I think my change broke 20 other unrelated existing tests, so I may have to think of a better way to accomplish the fix without affecting other stuff.

I have no idea if this makes sense whatsoever, because I don't
understand how this is supposed to work.
@msabramo
Copy link
Author

OK, I got all the tests passing, but honestly I feel like I was hacking around without really understanding what the behavior should be. So someone should take a close look and make sure I didn't mess things up.

let addedAutoVersion = false;

if (options.autoHelp && !options.flags.help) {
options.flags.help = {type: 'boolean'};
Copy link
Owner

Choose a reason for hiding this comment

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

You are modifying the user's input object here. You should not do that.

@@ -129,6 +129,19 @@ const meow = (helpText, options = {}) => {
...options,
};

let addedAutoHelp = false;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let addedAutoHelp = false;
let didAddAutoHelp = false;

@sindresorhus
Copy link
Owner

So someone should take a close look and make sure I didn't mess things up.

That someone is me and I don't think I'll time to review this in detail for quite some time, unfortunately.

msabramo added a commit to msabramo/chalk-cli that referenced this pull request Sep 27, 2021
Currently, `allowUnknownFlags: false` is commented out, which is not
ideal, because the error message the user gets when using an unknown
flag is pretty confusing:

```
$ chalk --foobar
Input required
```

The actual problem is in meow (see sindresorhus/meow#197), but it's been
taking a while to come up with a proper fix for that (see
sindresorhus/meow#198), so I suggest using this workaround in the
meantime.

See: chalk#32, chalk#33, chalk#39, sindresorhus/meow#197, sindresorhus/meow#198

Usage
foo <input>

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggested change

@sindresorhus
Copy link
Owner

Closing for lack of activity.

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.

--help and --version should work with allowUnknownFlags: false
3 participants