feat: rethink how options are inherited by commands #766
Conversation
|
@nexdrew, @JaKXz, @maxrimue, @lrlna this is getting much closer to being finished, and I would love some feedback. I've combined a lot of yargs' configuration logic into Along the way I've been cleaning up some API inconsistencies, and correcting a few bugs. For starters, it would be wonderful to have folks run this branch against some of their projects, specifically projects that use commands heavily (CC: @nexdrew I know you have a few). |
bcoe
changed the title
|
|
||
| // we apply validation post-hoc, to so that custom | ||
| // checks get passed populated positional arguments. | ||
| yargs._runValidation(innerArgv, aliases) |
There was a problem hiding this comment.
the little bit of refactoring in this method was to hold off on applying validation until after we've populated variadic values -- this was largely so that one can provide a .check() at the top level, and have it passed the appropriate post-processed object after commands are parsed.
| } | ||
| } | ||
|
|
||
| return setPlaceholderKeys(argv) | ||
| } | ||
|
|
||
| self._runValidation = function (argv, aliases) { |
There was a problem hiding this comment.
_runValidation was pulled into a helper, to make it easier to run optionally -- this was so that we could hold off on running it until the last moment in the case of commands.
| @@ -1112,9 +1107,9 @@ Options: | |||
| Missing required arguments: run, path | |||
| ``` | |||
|
|
|||
| <a name="demandCommand"></a>.demandCommand(min, [minMsg]) | |||
| <a name="demandCommand"></a>.demandCommand([min=1], [minMsg]) | |||
| if (!yargs._hasOutput()) populatePositionals(commandHandler, innerArgv, currentContext, yargs) | ||
|
|
||
| if (commandHandler.handler && !yargs._hasOutput()) { | ||
| commandHandler.handler(innerArgv) | ||
| } | ||
|
|
||
| // we apply validation post-hoc, to so that custom |
| @@ -418,6 +418,9 @@ Check that certain conditions are met in the provided arguments. | |||
| If `fn` throws or returns a non-truthy value, show the thrown error, usage information, and | |||
| exit. | |||
|
|
|||
| `global` indicates whether or not `check()` should be enabled both | |||
There was a problem hiding this comment.
nit: I think
indicates whether
check()should be enabled...
reads a little clearer, since I don't have to think about negations and my pyrate brain don't hurt.
| function populateParserHintArray (type, keys, value) { | ||
| keys = [].concat(keys) | ||
| keys.forEach(function (key) { | ||
| self.global(key) |
There was a problem hiding this comment.
Instead of explicitly marking everything as global, I kinda figured it would be easier to just invert the reset logic to assume that everything is global unless explicitly marked as non-global.
In that approach, we could perhaps change options.global to be options.local, treat yargs.global(opts, false) as a synonym for a new API method yargs.local(opts), and only worry about resetting local options in the reset method. (I think positional args for a command would be the only options to default as local.)
Thoughts? Is my view on the matter overly simplistic?
| @@ -596,6 +585,8 @@ function Yargs (processArgs, cwd, parentRequire) { | |||
| self.count(key) | |||
| } | |||
|
|
|||
| self.global(key, opt.global !== false) | |||
There was a problem hiding this comment.
1/2 If we stick with this approach, this needs to be the very last statement in the else block for this method; otherwise, whatever gets checked and called after it will make the option global again.
| }) | ||
| } else { | ||
| // a single key value pair 'x', parse() {} | ||
| self.global(key) |
There was a problem hiding this comment.
2/2 Similarly, this approach means that you must call .global(opts, false) last to disable an option from being global.
E.g. code like this will not have the desired effect because the call to .coerce() makes the option global again:
require('yargs')
.describe('opt', 'My local option')
.string('one')
.global('opt', false)
.coerce('opt', opt => 'local_' + opt)
.argvThis is probably not a big deal, but we should either mention this caveat in the README/docs or go with another approach that doesn't have this caveat.
BREAKING CHANGE: by default options, and many of yargs' parsing helpers will now default to being applied globally; such that they are no-longer reset before being passed into commands.
BREAKING CHANGE: by default options, and many of yargs' parsing helpers will now default to being applied globally; such that they are no-longer reset before being passed into commands.
BREAKING CHANGE: by default options, and many of yargs' parsing helpers will now default to being applied globally; such that they are no-longer reset before being passed into commands.
This is to address the fact that the existing behavior has created a lot of confusion: #659, #762, #764, etc.
This is going to be a pretty major change, and would appreciate as much feedback as possible as it's built out.
@nexdrew I'm going to merge your work from #734 into this.