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

isMultiple string flag is matching also the input arguments #160

Closed
dfernandez79 opened this issue Sep 18, 2020 · 17 comments · Fixed by #162
Closed

isMultiple string flag is matching also the input arguments #160

dfernandez79 opened this issue Sep 18, 2020 · 17 comments · Fixed by #162

Comments

@dfernandez79
Copy link

I have a flag (called multiple) with a string type and the isMultiple flag. When I use that flag with an input argument, the input argument is now part of the flag values. This doesn't happen if you put the flag at the end.

Example code

Test repo: https://github.com/dfernandez79/ismultiple-flag-example

const meow = require('meow');
const cli = meow('This is a test. Try --multiple=string inputArg', {
  flags: {
    multiple: {
      type: 'string',
      isMultiple: true,
    }
  }
});

console.log('Input: ', cli.input);
console.log('Multiple: ', cli.flags.multiple)

Outputs

➜ node . --multiple=string inputArg
Input:  []
Multiple:  [ 'string', 'inputArg' ]
➜ node . inputArg --multiple=string
Input:  [ 'inputArg' ]
Multiple:  [ 'string' ]
@sindresorhus
Copy link
Owner

// @ulken

@ulken
Copy link
Contributor

ulken commented Sep 19, 2020

Haven't looked at the code, but as I recall we allow (isMultiple) values to be space separated, which is why inputArg is swallowed by the flag.

As I see it, we have two options:

  1. Introduce a breaking change by not allowing space (only comma) as a separator.
  2. Force users to explicitly use -- to distinguish flag values from input args.

How do we want this to work?

@ulken
Copy link
Contributor

ulken commented Sep 19, 2020

Not sure if it would work, but maybe require wrapping the argument in quotes if one wants to space separate values? Then we could only consume the first argument and split it on whitespace.

@ulken
Copy link
Contributor

ulken commented Sep 19, 2020

I won't have a look at it until it's been decided what the desired behavior is here.

@dfernandez79
Copy link
Author

When I submitted the bug, I didn't know that this was the expected behavior. It seems that yargs works in a similar way with array flags.

I can workaround my CLI app by splitting a single value flag.

I understand if there is no desire to change it. As user of the CLI I found this behavior to be unexpected, because double dash usually means do not parse the rest. My expectation for the multiple flag was something like: --flag=val1 --flag=val2 or --flag=val1,val2.

It's ok for me to close the bug, if the behavior is by design.

@ulken
Copy link
Contributor

ulken commented Sep 20, 2020

Yes, you're right regarding --, so that's not a valid solution.

The examples you list should still work though. Or are you saying it parses inputArg even though you use comma-separated syntax? If so, then that's a bug 🐛.

@ulken
Copy link
Contributor

ulken commented Sep 22, 2020

@sindresorhus What say you? Change or close?

@sindresorhus
Copy link
Owner

but as I recall we allow (isMultiple) values to be space separated

I must have missed that when merging the PR. I agree, with @dfernandez79 that it's not what the user would expect, and neither would I.

@sindresorhus
Copy link
Owner

sindresorhus commented Sep 24, 2020

Introduce a breaking change by not allowing space (only comma) as a separator.

👍🏻 I think it's worth doing a major release to fix this.

@ulken
Copy link
Contributor

ulken commented Sep 24, 2020

I must have missed that when merging the PR. I agree, with @dfernandez79 that it's not what the user would expect, and neither would I.

Then I apologize for introducing it. No excuse, but I got it from one of the previous PR attempts.

@sindresorhus
Copy link
Owner

@ulken I was not blaming you, FWIW.

@ulken
Copy link
Contributor

ulken commented Oct 1, 2020

Fixed the first half of the issue, i.e. not greedily consume array flag arguments. I'll make a separate PR for supporting comma-separated values.

@ulken
Copy link
Contributor

ulken commented Oct 1, 2020

Gave it a go, but did not end up well. I could easily make it work for string-array, but it would be unintuitive to only support that.

I used the coerce option to split array flag arguments on ,, which works fine for strings. Unfortunately, for number-array and boolean-array the values have already been type coerced when passed to the coerce function (yielding NaN for numbers and true for booleans) and thus there's no , left to split on...

@ulken
Copy link
Contributor

ulken commented Oct 1, 2020

Open issue in yargs to support comma-separated values: yargs/yargs#846

@ulken
Copy link
Contributor

ulken commented Oct 1, 2020

If we really want to support comma separated values, I suppose we could convert all array types to string and then manually coerce them to number or boolean.

@ulken
Copy link
Contributor

ulken commented Oct 1, 2020

No, that's not a viable option either. It ends being really brittle having to handle defaults etc. (apart from handling type coercion).

@sindresorhus
Copy link
Owner

If we really want to support comma separated values, I suppose we could convert all array types to string and then manually coerce them to number or boolean.

Nah. We can wait for Yargs to implement it: #164

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

Successfully merging a pull request may close this issue.

3 participants