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

Add support for getting merged options including globals #1671

Merged
merged 7 commits into from
Jan 11, 2022

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jan 4, 2022

Pull Request

Problem

Currently no direct support for getting merged options including globals. The work-around is to access the global options on the program or command parent.

This was the second most popular enhancement in poll #1551 (comment)

Related issues: #243 #476 (comment) #1024 #1229

Previous consideration by me: #1229 (comment) #1155 #1478

Solution

Add mergeOptions parameter to `.opts()`.
const options = cmd.opts();
const mergedOptions = cmd.opts({ includeGlobals: true });

Add command.optsWithGlobals().

Example Usage

const { program } = require('commander');

program
  .option('-g, --global');

program
  .command('sub')
  .option('-l, --local')
  .action((options, cmd) => {
    console.log({
      opts: cmd.opts(),
      optsWithGlobals: cmd.optsWithGlobals())
    });
  });

program.parse();
$ node index.js sub --global --local
{
  opts: { local: true },
  optsWithGlobals: { local: true, global: true }
}

ChangeLog

  • add .optsWithGlobals() to return merged local and global options

@kuncevic
Copy link

kuncevic commented Jan 6, 2022

Oh, BTW, why wouldn't { includeGlobals: true } became a default behavior?

@shadowspawn
Copy link
Collaborator Author

Option 2 in the poll was a configuration to always include the globals in the options, but it didn't get a lot of likes.

@kuncevic
Copy link

kuncevic commented Jan 6, 2022

Yeah i c, in that case you had to call that program.includeGlobalsInOptions(); however you may want to pass more params to .opts({ includeGlobals: true, more_params }) right?

@shadowspawn
Copy link
Collaborator Author

Yes. Commander has some routines where the boolean arg is optional because the method name is clear about what will happen:

program
  .allowUnknownOption()
  .storeOptionsAsProperties();

I don't like adding a plain boolean parameter which has no obvious meaning, like say:

program.opts(true);

Adding an options parameter makes effect clearer by naming the value, especially when not commonly used, and also allows future expansion.

program.parse(['--debug'], { from: 'user' });
program.help({ error: true });
program.command('foo', 'foo description', { isDefault: true });

@kuncevic
Copy link

kuncevic commented Jan 6, 2022

Yeah, plain boolean is definitely no go. However speakingprogram.includeGlobalsInOptions(); vs .opts({ includeGlobals: true }) as you already have things like .allowUnknownOption() having .includeGlobalsInOptions(); seems quite natural.

@shadowspawn
Copy link
Collaborator Author

Yes I agree .includeGlobalsInOptions() flows better than .opts({ includeGlobals: true }).

Using a separate method would look like:

//const mergedOptions = cmd.cmd.opts({ includeGlobals: true })
const mergedOptions = cmd.optsWithGlobals();

Do you like that better?

(Or are you now thinking option 2 is better!?)

@kuncevic
Copy link

kuncevic commented Jan 8, 2022

Yeah, cmd.optsWithGlobals(); looks good, it seems more natural and really flows better along with .allowUnknownOption(), etc.

@shadowspawn
Copy link
Collaborator Author

Switched to .optsWithGlobals in PR description and code.

Copy link

@kuncevic kuncevic left a comment

Choose a reason for hiding this comment

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

That is great. Looking forward to see it pushed in the next release 🔥

@shadowspawn shadowspawn marked this pull request as ready for review January 10, 2022 05:27
lib/command.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

LGTM!

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jan 11, 2022
@shadowspawn shadowspawn added this to the Commander v9.0.0 milestone Jan 11, 2022
@shadowspawn
Copy link
Collaborator Author

Thanks @kuncevic and @abetomo

@shadowspawn shadowspawn merged commit 772eb53 into tj:release/9.x Jan 11, 2022
@shadowspawn shadowspawn deleted the feature/opts-with-globals branch January 11, 2022 09:12
@shadowspawn
Copy link
Collaborator Author

.optsWithGlobals() is included in prerelease Commander v9.0.0-1.

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 29, 2022
@shadowspawn
Copy link
Collaborator Author

Commander v9 has been released.

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.

3 participants