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 help flag #33

Closed
wants to merge 2 commits into from
Closed

Fix help flag #33

wants to merge 2 commits into from

Conversation

msabramo
Copy link
Contributor

This fixes the issue of --help not working (and adds a test to prevent future breakages), mentioned in #32.

@sindresorhus
Copy link
Member

It's a bug in meow. I'm just going to disable strict flags until it's fixed. sindresorhus/meow#197

@msabramo
Copy link
Contributor Author

Oh interesting. So I shouldn't have to specify help in flags:

		help: {
			type: 'boolean'
		},

?

@msabramo
Copy link
Contributor Author

msabramo commented Sep 14, 2021

The problem with disabling strict flags, as you probably realized, is that now we're back to this, which is unfortunate:

$ node cli.js --foobar "I got your input right here"
Input required

@sindresorhus
Copy link
Member

@msabramo If you look at the meow docs, it's automatic.

@sindresorhus
Copy link
Member

The problem with disabling strict flags, as you probably realized, is that now we're back to this, which is unfortunate:

I don't want too many workarounds. It will hopefully be fixed in meow soon.

@msabramo
Copy link
Contributor Author

I don't want too many workarounds. It will hopefully be fixed in meow soon.

Yeah, I get that you want to fix things properly instead of having temporary bandaids that have to be removed later. But I guess I'd say that disabling strict flags is its own workaround and it's one that unfortunately has an impact on the user experience. And it also would ideally be removed when meow is fixed.

So whatever you want to do is fine of course, but I'd suggest this:

--- a/cli.js
+++ b/cli.js
@@ -54,9 +54,12 @@ const cli = meow(`
          $ echo 'Unicorns from stdin' | chalk --stdin red bold
 `, {
        importMeta: import.meta,
-       // TODO: Disabled until https://github.com/sindresorhus/meow/issues/197 is fixed.
-       // allowUnknownFlags: false,
+       allowUnknownFlags: false,
        flags: {
+               // TODO: Can be removed when https://github.com/sindresorhus/meow/issues/197 is fixed.
+               help: {type: 'boolean'},
+               version: {type: 'boolean'},
+
                template: {
                        type: 'string',
                        alias: 't',

This version handles both --help and --version (I didn't realize --version was a flag until I saw the meow bug you referenced) and I added a tweaked version of your TODO comment, so that folks know why this weirdness is there and that it can hopefully be removed soon.

msabramo added a commit to msabramo/chalk-cli that referenced this pull request Sep 27, 2021
msabramo added a commit to msabramo/chalk-cli that referenced this pull request Sep 27, 2021
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 meow#197), but it's been taking a
while to come up with a proper fix for that (see meow#198), so I suggest
using this workaround in the meantime.

See: chalk#32, chalk#33, chalk#39, meow#197, meow#198
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
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