feat: configuration setting for running detailed parse #1472
Conversation
There was a problem hiding this comment.
The feature itself looks interesting, but I'm uneasy about the quantity of new ifs this feature brings, and the increased complexity of command.js and yargs.js afterwards.
In particular, this is hard to believe, when reading tests, that so much different preprocessing before calling command handlers on the first hand, and before returning from yargs.parse() on the other hand, leads to the same results... I hope we will never have issues there :-)
This reminds me of the debate about having a parallel detailed argv structure a few months ago, which we never did because we were afraid of adding too much complexity...
| if (!detailedPositionalParse[key]) { | ||
| detailedPositionalParse[key] = { | ||
| value: parsed.argv[key], | ||
| defaulted: !!parsed.defaulted[key] |
There was a problem hiding this comment.
If detailerPositionalParse's list of fields is intended to grow, we should not have to add these fields in 2 places.
I recommand refactoring this part.
|
|
||
| // short-circuit parse. | ||
| if (!unparsed.length) return | ||
| if (!unparsed.length) return detailedPositionalParse |
There was a problem hiding this comment.
In that case, postProcessPositionals (now runParserOnPositionals) used to return nothing and populatePositionals (now addPositionalsToArgv) used to return positionalMap.
Now, runParserOnPositionals returns {} and addPositionalsToArgv returns it too. Isn't something missing? Shouldn't detailedPositionalParse contain at least values from positionalMap before being returned?
|
@mleguen I agree about concerns of adding complexity, however I also feel we need some way to expose a tldr; I think this ultimately is actually a good replacement to |
| }, [(argv) => { | ||
| return { | ||
| hello: 'world' | ||
| } | ||
| }]) |
There was a problem hiding this comment.
What is this final callback argument? Seems like this test could do without it - also with detailed: true I'd expect this callback to use ({argv}) argument (parse.argv instead of just argv).
There was a problem hiding this comment.
this is middleware, which can be passed as an array as the final argument to a command. The idea with middleware is that it might be a third-party plugin that someone has written, so I think the API surface should continue getting argv (otherwise someone writing middleware would need to detect a detailed parse, vs., a regular parse).
| parse.argv.identifier.should.equal('abc123') | ||
| parse.argv.foo.should.equal('bar') | ||
| parse.argv.banana.should.equal('yellow') | ||
| parse.argv.hello = 'world' |
There was a problem hiding this comment.
Setting parse.argv.hello seems unneeded?
| .option('banana', { | ||
| default: 'yellow' | ||
| }) | ||
| .parse('foo abc123 --apple red') |
There was a problem hiding this comment.
As with the yargs-parser tests it would be good if we could use --apple green to show that yargs.defaulted.apple is cleared when the default value is explicitly set to the default value by the user. Also missing a test above to verify parse.defaulted.apple is unset.
| default: 'blerg' | ||
| }) | ||
| .positional('foo', { | ||
| default: 'bar' |
There was a problem hiding this comment.
What is the expected behavior when a default is not configured? If you remove this default: 'bar' would this mean parse.defaulted.foo == true && parse.argv.foo === undefined?
| parse.argv.id.should.equal('abc123') | ||
| parse.argv.identifier.should.equal('abc123') | ||
| parse.argv.foo.should.equal('bar') | ||
| parse.argv.banana.should.equal('yellow') |
There was a problem hiding this comment.
Need to check argv.apple and defaulted.apple.
|
@coreyfarrell is there a more minimal API surface that would work for your use-case, maybe we could just get away with introducing |
For my use case |
|
we have this ancient pull request as well, asking us to expose I don't love the idea of adding a yargs.isDefaulted(string) // returns true or false depending on whether the optoin was defaulted.☝️ I could imagine adding a handful of options like this. |
|
somewhat similar: #294, a way to filter out any keys that have aliases. |
|
The API of the |
|
What about the |
|
@coreyfarrell As a non-native English speaker, I am used to seeing (and writing) isXxxed functions for this kind of check. So I would recommend using isDefaulted. But that is only ONE non-native english speaker opinion... |
|
@mleguen @coreyfarrell I'm leaning towards the |
|
I need to actually sit down and work on the refactor discussed here, i.e., adding a helper method rather than a sweeping change to how parsing works based on a configuration setting. |
|
let's revisit this when someone has time, it would be good to expose the |
|
I think @coreyfarrell suggested something along the lines of a symbol on the |
You can now set
yargs.parserConfiguration({detailed: true})and receive detailed results fromyargs-parser.This provides an interface into @juergba's defaulted functionality.
fixes #1334
CC: @coreyfarrell.