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

feature: defaulted functionality #211

Merged
merged 4 commits into from
Nov 1, 2019
Merged

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Oct 28, 2019

closes #203

Parse.detailed() returns an additional object defaulted which contains any new argument created by opts.default. This argument has not been specified by the user.

defaulted: { foo: true }

defaulted does not include:

  • defaulted aliases
  • an option specified by the user on CLI/config/configObjects but without value

@bcoe
Copy link
Member

bcoe commented Oct 29, 2019

@juergba this feature is that simple? wow, I feel a bit silly for the complicated approach we'd left in the codebase for so long 😆 😢

@coreyfarrell mind taking a look? I believe you were a potential consumer of this functionality.

@bcoe bcoe requested a review from mleguen October 29, 2019 00:58
@juergba
Copy link
Contributor Author

juergba commented Oct 29, 2019

This draft PR is not ready for review, yet. Please note:

  • defaulted aliases are not logged
  • an option specified on CLI/config/configObjects but without value, is not logged

@coreyfarrell
Copy link
Contributor

Quick glance this looks good. Please comment when you mark this ready for review to make sure github sends notifications.

@juergba juergba marked this pull request as ready for review October 30, 2019 09:15
@juergba
Copy link
Contributor Author

juergba commented Oct 30, 2019

Please comment when you mark this ready for review to make sure github sends notifications.

Ok, ready for review, thanks.

* `configuration`: the configuration loaded from the `yargs` stanza in package.json.
* `newAliases`: any new aliases added via camel-case expansion:
* `boolean`: `{ fooBar: true }`
* `defaulted`: any new argument created by `opts.default`, no aliases included.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juergba what's your reasoning for not including aliases? @coreyfarrell do you think you can work with this limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parse.detailed() also returns aliases which includes all aliases, so the information would be redundant.
It's no big effort to add the aliases to defaulted, I just need a decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my use it would be fine if defaulted did not list aliases, as long as setting an alias causes the original key to be excluded from defaulted. Would you mind adding a test for this?

    it('setting an alias removes associated key from defaulted', function () {
      var parsed = parser.detailed('--foo abc', {
        default: { kaa: 'abc' },
        alias: { foo: 'kaa' }
      })
      parsed.argv.should.deep.equal({ '_': [], kaa: 'abc', foo: 'abc' })
      parsed.defaulted.should.deep.equal({})
    })

Sorry I don't have a chance to actually run this test right now but in theory it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I added your test.

Copy link
Contributor

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @juergba!

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for this contribution, I think we should hold off on alias support until it's needed, talking with @coreyfarrell I think what we might do is introduce the feature parseDetailed to yargs, which would have the same output as a detailed parse in yargs-parser, for the folks that need defaulted functionality like nyc I think this will be a good start.

@bcoe bcoe merged commit a525234 into yargs:master Nov 1, 2019
@juergba
Copy link
Contributor Author

juergba commented Nov 2, 2019

I think what we might do is introduce the feature parseDetailed to yargs, which would have the same output as a detailed parse in yargs-parser, [...]

Yargs.parsed:
if I understand the Yargs docu correctly, then parsed should include defaulted. I haven't tested it though.

@bcoe
Copy link
Member

bcoe commented Nov 5, 2019

@juergba mind taking a look at yargs/yargs#1472.

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

Successfully merging this pull request may close these issues.

feat: add back defaulted functionality
3 participants