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 check for implies argument type #1852

Closed
canopy-js-user opened this issue Mar 9, 2023 · 7 comments
Closed

Add check for implies argument type #1852

canopy-js-user opened this issue Mar 9, 2023 · 7 comments

Comments

@canopy-js-user
Copy link

canopy-js-user commented Mar 9, 2023

Hi there, twice I've accidentally written a line of code like this:

.addOption(new Option(
    '--port <number>', 
    'Which port to run the server on for sync mode'
).default(undefined).implies('sync'))

Ie, I want the --port option to imply the boolean flag sync. However, sync is expecting an object of implied key-value pairs. However, as is, this fails silently and converts the string/array 'sync' into an object, and merges it with the rest of the options:

{
  '0': 's',
  '1': 'y',
  '2': 'n',
  '3': 'c',
  editor: true,
  logging: true,
  open: true,
  backup: true,
  sync: true,
  port: '4004'
}

I'm proposing a check that the argument to implies is an object and not a string, and if it is a string, error. (Or, interpret the string as the name of a boolean option that the programmer is asserting should be true.)

Thanks!

@canopy-js-user
Copy link
Author

canopy-js-user commented Mar 10, 2023

(Another reason I think this an easy mistake is that many of the option methods do take strings, eg .conflicts. Thanks!)

@shadowspawn
Copy link
Collaborator

Commander does not have a lot of runtime checks on the parameters, but I am happy to consider adding checks or improving behaviour for routines that are easy to get wrong.

(A past one that was added was #1655.)

@canopy-js-user
Copy link
Author

Makes sense, thanks!

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Mar 15, 2023
@shadowspawn shadowspawn changed the title Add check for impies argument type Add check for implies argument type Apr 9, 2023
@shadowspawn
Copy link
Collaborator

For interest, we added support in .conflicts() for a string or an array after making a similar mistake during development: #1678 (comment)

@canopy-js-user
Copy link
Author

@shadowspawn Very nice!

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Apr 15, 2023
@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 15, 2023

Shipped #1854 in 10.0.1

Thanks @canopy-js-user

@canopy-js-user
Copy link
Author

@shadowspawn thank you!

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

No branches or pull requests

2 participants