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

Conflicting options with default values (or boolean values) #929

Closed
jordaaash opened this issue Jul 28, 2017 · 25 comments
Closed

Conflicting options with default values (or boolean values) #929

jordaaash opened this issue Jul 28, 2017 · 25 comments

Comments

@jordaaash
Copy link

jordaaash commented Jul 28, 2017

Using conflicts with an option that has a default value, or a boolean option (which is automatically set to false even if a default value is not provided) always throws an error, because the conflicting option name will always be defined.

@john-kurkowski
Copy link

Similar problem with variadic positional arguments. Maybe because those have a default value of []?

yargs('my-command --update-all')
  .command('my-command [things-to-update..]', 'Some description', {
    'update-all': {
      boolean: true,
      conflicts: 'things-to-update',
    },
  }, function(argv) {
    console.log(argv['update-all']);
  })
  .argv;

Expected: log true.
Actual: exit 1, Arguments update-all and things-to-update are mutually exclusive.

@bcoe
Copy link
Member

bcoe commented Sep 17, 2017

@jordansexton any thoughts about what the default behavior should be ... what's weird is that false is a value it feels like it would be too loose to have the conflicts logic simply look for a falsy value since values like "" would also pass the conflicts test.

@jordaaash
Copy link
Author

Yeah, special preference for false or falsy values would be hard to be consistent/predictable with. However, a default value shouldn't inherently conflict just by being declared. I think the expected behavior would separate the concept of a conflicting value from default values.

This might be accomplished by evaluating conflicting values that are provided as args, and then applying defaults. Alternatively, it could apply defaults/coercion/etc. and then evaluate conflicting values as being values not equal to the respective default values if provided.

require('yargs')
    .options({
        i: {
            alias:     'include',
            default:   false,
            describe:  'Include something',
            type:      'boolean',
            conflicts: 'exclude'
        },
        e: {
            alias:     'exclude',
            default:   false,
            describe:  'Exclude something',
            type:      'boolean',
            conflicts: 'include'
        }
    });

I read the intent of the above option declaration is to say "either one can be true, but not both".

script                       # No args, apply defaults. No error.
script --include             # Include is true, exclude is default (false). No error.
script -i false -e false     # Superfluous, but both match their defaults. No error.
script --include --exclude   # Both aren't identical to their defaults. Error!

This way, conflicts regards default values, or provided values that match default values, as non-conflicting, satisfying the intent without a preference for a particular kind of value.

I haven't looked to see how difficult this implementation would be, but what do you think of the reasoning?

@bcoe
Copy link
Member

bcoe commented Sep 17, 2017

@jordansexton I agree with what you're saying ... I don't think the fact that we've applied a default value for an argument should result in it being flagged as a conflict.

Because the default values are applied in the parsing library yargs-parser and the conflicts logic is applied in yargs itself, the actual implementation might be a bit of a pain ... I think your second suggestion would work for most scenarios, if it's a value other than default it conflicts.

Do you have any interest in taking a stab at implementing this?

@jordaaash
Copy link
Author

Yeah, I'll take a look at the code and see if I can get it done.

@jordaaash
Copy link
Author

@bcoe So, defaults are applied in yargs-parser, which should be okay for what I'm thinking, provided that yargs itself can see the defaults where it would need to @

yargs/lib/validation.js

Lines 324 to 333 in 0e0c58d

Object.keys(argv).forEach((key) => {
if (conflicting[key]) {
conflicting[key].forEach((value) => {
// we default keys to 'undefined' that have been configured, we should not
// apply conflicting check unless they are a value other than 'undefined'.
if (value && argv[key] !== undefined && argv[value] !== undefined) {
usage.fail(__(`Arguments ${key} and ${value} are mutually exclusive`))
}
})
}

Can it?

@jordaaash
Copy link
Author

Okay, so it looks like https://github.com/yargs/yargs-parser/blob/00bde7d60c433c188f2529a33bab187d40645f7d/index.js#L677-L683 would need to change to include flags.defaulted and/or defaults in the returned object.

Then

yargs/yargs.js

Lines 1063 to 1073 in 0e0c58d

self._runValidation = function runValidation (argv, aliases, positionalMap, parseErrors) {
if (parseErrors) throw new YError(parseErrors.message)
validation.nonOptionCount(argv)
validation.missingArgumentValue(argv)
validation.requiredArguments(argv)
if (strict) validation.unknownArguments(argv, aliases, positionalMap)
validation.customChecks(argv, aliases)
validation.limitedChoices(argv)
validation.implications(argv)
validation.conflicting(argv)
}
and its call location a few lines up would need to be changed to pass argv as well as the defaults to the code from #929 (comment) which can then compare against the value rather than the key.

If this sounds right to you, I'll first open a PR against yargs-parser for the first change and then follow up here.

@jordaaash
Copy link
Author

I'm considering that Parser.detailed should just return all the determined flags and so on, and just let the caller decide what it cares about. Treating flags.defaults as a special case for this feature seems weird. And then runValidation can be changed to a signature of (argv, parsed, ...) or (argv, flags, ...) and cherry pick aliases, defaults, and whatever else each individual validation method might care about.

@daBGHarry
Copy link

daBGHarry commented Sep 28, 2017

Hey everyone. I don't know of how much use this sample will be, but it seems booleans and default values aren't the only affected types. The code below is in NodeJs, and the javascript file is called test.js

const yargs = require('yargs');

const argv = yargs
    .usage('<cmd> args')
    .command('hello', 'the best command', {
        alias: 'h',
        describe: 'Says hello'
    }, function (argv) {
        console.log(argv);
    })
    .option('work', {
        alias: 'w',
        describe: 'does some work',
        conflicts: 'run'
    })
    .option('run', {
        alias: 'r',
        describe: 'runs something',
        conflicts: 'work'
    })
    .help()
    .argv;

Executing node test.js hello --w yields Arguments work and run are mutually exclusive

EDIT: Embarrassing... I was also using version 8.0.2. Updating to 9.0.1 fixed the issue. Thank you @quilicicf

@quilicicf
Copy link

quilicicf commented Oct 2, 2017

Hi I've bumped into the same issue as @zzharryzz and resorted to using the check method instead.
I'd be willing to contribute if it helps, just tell where to look!

EDIT: my bad, works like a charm with version 9.0.1 (was using 8.0.2).

@alexreg
Copy link

alexreg commented Oct 12, 2017

I still get this problem on 9.0.1... it seems boolean options always conflict.

@quilicicf
Copy link

quilicicf commented Oct 14, 2017

Yup, my problem with string/integer parameters vanished with version 9 but there's still an issue with booleans.

Check out this code for an example.
I tried adding argument from-base-branch to the list of conflicting arguments on argument commits-number but it always results in conflicts.

@RyanZim
Copy link

RyanZim commented Dec 6, 2017

Just ran into this bug now.

For now, I'm setting coerce: (v) => v || undefined on the boolean flag that's causing the problem as a workaround.

@ljharb
Copy link
Contributor

ljharb commented Dec 7, 2017

I'm running into this issue as well; it makes it impossible to have mutually exclusive options where one or both are booleans.

@bcoe what I would expect is that if the argument is a boolean, it only considers it "set" if it's true. Similarly, conflicts should be checked before default values are applied.

@gombosg
Copy link

gombosg commented Mar 5, 2018

Bug still confirmed for yargs 11. Options always conflict with boolean arguments (with a 'false' default value).
Non-boolean arguments are fine.
Workaround is to disable the boolean type, often this doesn't cause any issues.
@jordansexton , did you come up with that PR?

@jordaaash
Copy link
Author

No, sorry, I haven't made any progress on this. I worked around it by just not using defaults for booleans, and then forgot to look into it beyond the comments above.

@MattMcFarland
Copy link

MattMcFarland commented Mar 22, 2018

I ran into a similar issue, and what fixed it for me was changing defaults to undefined...

    .option('debug', {
      alias: 'd',
      type: 'boolean',
      describe: 'logs all debug messages',
      conflicts: ['silent', 'quiet', 'verbose'],
      default: undefined
    })
    .option('quiet', {
      alias: 'q',
      type: 'boolean',
      describe: 'log only warnings',
      conflicts: ['silent', 'verbose', 'debug'],
      default: undefined
    })
    .option('silent', {
      alias: 's',
      type: 'boolean',
      describe: 'turn off logging',
      conflicts: ['quiet', 'verbose', 'debug'],
      default: undefined
    })
    .option('verbose', {
      alias: 'V',
      type: 'boolean',
      describe: 'enables verbose logging',
      conflicts: ['silent', 'quiet', 'debug'],
      default: undefined
    })

note above, without default: undefined I kept having the "arguments are mutually exclusive" problem

@FallingSnow
Copy link

Just ran into this issue. Two options that have a default of false and listed as conflicting causes Arguments encoder-preview and delete are mutually exclusive. Setting defaults to undefined instead of false does work but it removes the [default: false] from the --help output.

@plroebuck
Copy link

While I advocated not changing the existing setup (thinking about potential dependent package breakage issues), here's additional discussion in the yargs-parser concerning changing the default boolean to undefined.

@nborko
Copy link

nborko commented Jun 7, 2018

I am also running into this issue, but for type "count." In my case, I have a "debug" count option that increases the logging level, which should conflict with a "loglevel" option that explicitly sets the logging level. So, specifying loglevel always results in "Arguments loglevel and debug are mutually exclusive" even when debug is not specified (but has a default value of 0).

@RyanZim
Copy link

RyanZim commented Jul 18, 2018

This seems to be fixed in yargs v12.

@anentropic
Copy link

if it was fixed in yargs 12 it is back in 13.3.0

two boolean options with default false are conflicting when un-set

@bcoe
Copy link
Member

bcoe commented Jul 23, 2019

@anentropic would you be able to provide a minimal example of the bug in action, we should put a test around this if it was a regression.

@anentropic
Copy link

sure, my code is basically:

/**
 * @param {Array<string>} rawArgs - i.e. from `process.argv`
 * @returns {Object} parsed args
 */
export function parseArgs(rawArgs) {
  return yargs
    .command('$0 <files...>', 'My tool command', (yargs) => {
      yargs
        .option('no-color', {
          alias: 'n',
          type: 'boolean',
          default: undefined,  // default: false triggers conflict warning by yargs
          describe:
            `Monochrome output only`
        })
        .option('force-color', {
          alias: 'c',
          type: 'boolean',
          default: undefined,  // default: false triggers conflict warning by yargs
          conflicts: 'no-color',
          describe:
            `When running as a pre-commit hook we are unable to detect if the ` +
            `terminal supports colored output. If you want to see colored ` +
            `output then set this flag.`
        });
    })
    .help()
    .parse(rawArgs.slice(2))
  ;
}

@bcoe bcoe added the p1 label Jul 24, 2019
@mleguen
Copy link
Member

mleguen commented Aug 28, 2019

This issue is solved now for booleans: a conflict will be triggered only if you explicitely set a default value (as for other types).

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

No branches or pull requests