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

opts collisions #2260

Open
mixmix opened this issue Oct 2, 2024 · 6 comments
Open

opts collisions #2260

mixmix opened this issue Oct 2, 2024 · 6 comments

Comments

@mixmix
Copy link

mixmix commented Oct 2, 2024

Problem: if you specify the an option with the same name at multiple levels, you get unpredictable results.

This is probably developer error, but it could be an edge case worth documenting (here as an issue) or something to proactively check for.

test:

import test from 'tape'
import { Command, Option } from 'commander'

test('too many opts on the dance floor', async (t) => {
  const accountOption = () => {
    return new Option('-a, --account <name|address>')
      .argParser((str) => str.toUpperCase())
      .default('DERP')
  }

  const program = new Command()
    .addOption(accountOption())

  const danceCommand = new Command('dance')
    .addOption(accountOption())
  program.addCommand(danceCommand)

  const input = 'dance --account naynay'
  await program.parseAsync(input.split(' '), { from: 'user' })

  t.deepEqual(program.opts(), { account: 'NAYNAY' })
  // ✓

  t.deepEqual(danceCommand.opts(), { account: 'NAYNAY' })
  // ✗ gets { account: 'DERP' }

  t.end()
})

I discovered this because I was seeing the default('DERP') value arrive subcommand action even when I had explicitly set the --account flag. I initially thought there was collision / race condition happening between argParser and default, because logging inside argParser showed that that was being called. (keyword sprinkling to help others searching for this error)

I think this is happening because command is generously allowing options to be anywhere positionally (I saw mention in another issue this can be disabled).

Propose:
if options can be inserted anywhere as a default, add a check that there are not duplicate options that would collide up and down the command/subcommand tree

@mixmix
Copy link
Author

mixmix commented Oct 3, 2024

Thinking more... there are some times when there is a command which might take an option, and it also has a subcommand which would also take that option

e.g.

git-help status --endpoint https://api.gitlab.com
git-help status issues --endpoint https://api.gitlab.com

To set that up I would write something like

const endpointOption = new Option('-e, --endpoint <url>')
  .default('http://api.github.com/')

const program = new Command()
  .name('git-help')
  .addCommand(statusCommand())

function statusCommand () {
  const status = new Command('status')
    .addOption(endpointOption)
    .action(async () => {})

  status
    .addCommand(
       new Command('issues')
         .addOption(endpointOption)
         .action(async () => {})
     )
  return status
}

I need to specify the option at multiple levels here so that it's in the --help
I want the same option for consistency
I also expect (here) the option to be relevant to the lowest-level subcommand.

hmmm is this just a badly designed CLI?

@shadowspawn
Copy link
Collaborator

Thanks for the clear description and example code.

Some quick clues ((I'll post a longer reply later). Take a look at:

  • allowing distinct options at multiple levels despite same name: .enablePositionalOptions()
  • displaying single option at multiple levels: showGlobalOptions in .configureHelp()

@shadowspawn
Copy link
Collaborator

It is intended that program options are recognised anywhere on command-line. Selected quotes from README:

  • By default, options on the command line are not positional, and can be specified before or after other arguments.

  • By default, program options are recognised before and after subcommands.

Global options can be a bit more complicated for the author and end user, but there are a few additions specifically to help.

You can access program options by calling program.opts() or by using cmd.optsWithGlobals() from a subcommand. To have the global options visible in the help from a subcommand, see showGlobalOptions in the README.

If you do want to have the same named option on the program and on a subcommand and both be usable, you can make the options positional. See enablePositionalOptions().

@mixmix
Copy link
Author

mixmix commented Oct 23, 2024

  • enablePositionalOptions - this seems like a foot gun IMO
  • showGlobalOptions - this seems useful to know about!

You can access program options by calling program.opts() or by using cmd.optsWithGlobals() from a subcommand.

AH, this could be a good! I found reasoning about this pretty hard, so ran some tests to make this clearer:

import test from 'tape'
import { Command, Option } from 'commander'

test('too many opts on the dance floor', async (t) => {
  const accountOption = () => {
    return new Option('-a, --account <name|address>')
      .argParser((str) => str.toUpperCase())
      .default('DERP')
  }

  const program = new Command()
    .addOption(accountOption())
    .action(opts => {               // root_action
      console.log(opts)
      console.log(program.opts())
    })

  const danceCommand = new Command('dance')
    .addOption(accountOption())
    .action(opts => {               // dance_action
      console.log(opts)
      console.log(program.opts())
      console.log(danceCommand.opts())
      console.log(danceCommand.optsWithGlobals())
    })

  program.addCommand(danceCommand)

  const inputA = '--account naynay'
  await program.parseAsync(inputA.split(' '), { from: 'user' })
  // logging from root_action reveals:

  // opts           => { account: 'NAYNAY' })
  // program.opts() => { account: 'NAYNAY' })


  const inputB = 'dance --account naynay'
  await program.parseAsync(inputB.split(' '), { from: 'user' })
  // logging from dance_action reveals:

  // opts                           => { account: 'DERP' }
  // program.opts()                 => { account: 'NAYNAY' }
  // danceCommand.opts()            => { account: 'DERP' }
  // danceCommand.optsWithGlobals() => { account: 'NAYNAY' }

  t.end()
})

Honestly I find the results surprising / I don't know if I'm confident enough in the results to be able to safely know which opts to be using ... unless I test each option against the default and look for any deviations?

@shadowspawn
Copy link
Collaborator

Why do you have the same named option at two different levels? Can you give some examples of how you intend to call the program from the command-line?

I don't currently understand what you are trying to achieve.

@shadowspawn
Copy link
Collaborator

To put it another way, are the program options meant to apply to all subcommands? If the options are just for the program it sometimes makes sense to move the functionality into a subcommand and make it the default command. You can call it the same way, but the refactor shifts the "program" down a level so it is alongside the other subcommands and the options are independent.

See: #1616 #1750

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

No branches or pull requests

2 participants