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

Providing both an --option and a --no-option variant causes the option to always be boolean. #34

Closed
spiltcoffee opened this issue Nov 30, 2018 · 8 comments
Labels

Comments

@spiltcoffee
Copy link

Providing both an --option and a --no-option variant causes the option to always be boolean.

This means even if --option takes in a [value] or , since --no-option is defined, it will only ever be true or false, and specifying a value causes it to be interpreted as an argument instead.

Example repo: https://github.com/spiltcoffee/cac-no-example

@egoist egoist closed this as completed in b1fbe18 Nov 30, 2018
@egoist
Copy link
Collaborator

egoist commented Nov 30, 2018

🎉 This issue has been resolved in version 6.3.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@spiltcoffee
Copy link
Author

@egoist I'm not sure if b1fbe18 fixes the whole thing.

Looking at vuepress for a second, we have this block of code defining the cli... (take note of the --cache and --no-cache options)

https://github.com/vuejs/vuepress/blob/master/packages/%40vuepress/cli/index.js#L40-L48

... and then we use it here...

https://github.com/vuejs/vuepress/blob/master/packages/%40vuepress/core/lib/prepare/CacheLoader.js#L20-L28

The usage of the cache option here suggests that the command line is meant to work as such:

  • --cache [cache]: non-empty string
  • --cache: true
  • --no-cache: false

If I've understood the changes in the commit correctly, what we get now is this:

  • --cache [cache]: non-empty string
  • --cache: empty string
  • --no-cache: false

I'm not sure if the usage is incorrect or if this package isn't quite doing things right yet.

@egoist
Copy link
Collaborator

egoist commented Nov 30, 2018

I guess you're right

@egoist egoist reopened this Nov 30, 2018
@egoist egoist closed this as completed in ca1b5bb Nov 30, 2018
@egoist
Copy link
Collaborator

egoist commented Nov 30, 2018

🎉 This issue has been resolved in version 6.3.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@egoist
Copy link
Collaborator

egoist commented Nov 30, 2018

Hopefully it's fixed now.

However seems there's another issue with the usage in VuePress:

Since you used cli.option('--no-cache') here, its default value will be set to true. So in this line it will never check siteConfig.cache

Maybe you should use the ignoreOptionDefaultValue option:

cli.command('foo', 'desc', { ignoreOptionDefaultValue: true })

/cc @ulivz

@spiltcoffee
Copy link
Author

spiltcoffee commented Nov 30, 2018

That should be okay, because I think we do look at siteConfig.cache if --no-cache is used then?

I dunno, probably need ulivz to look at the logic here and figure out if the command line options and the logic here still matches up.

@egoist
Copy link
Collaborator

egoist commented Nov 30, 2018

@spiltcoffee well at least this is the same in commander.js

@ulivz
Copy link
Contributor

ulivz commented Dec 3, 2018

Cool~ ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants