Skip to content

Conversation

@shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jun 7, 2020

Pull Request

One day we might change the default behaviour to make options safer! In the meantime, we can try harder to warn about possible problems.

Problem

While the default behaviour is to store options values as properties, people may hit issues with clashes with existing properties:

Collected issues: #933
Recent --name: #1226 #1282
Recent --no-emit: #1237

Solution

Throw an error if the user has not called storeOptionsAsProperties and the property matching the option is already defined.

Takes a reasonable amount of code to avoid incorrect warnings, but hopefully helps programmer resolve the problem.

ChangeLog

  • throw an error with advice on how to resolve if there might be a clash between option name and a Command property

Example

program
  .option('-E,--no-emit')
...
      throw new Error(`option '${option.flags}' clashes with existing property '${option.attributeName()}' on Command
      ^

Error: option '--no-emit' clashes with existing property 'emit' on Command
- call storeOptionsAsProperties(false) to store option values safely,
- or call storeOptionsAsProperties(true) to suppress this check,
- or change option name
...

@shadowspawn
Copy link
Collaborator Author

Adding as draft for a few days in case I think of more cases to cover.

@shadowspawn shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Jun 7, 2020
@shadowspawn shadowspawn changed the title WIP: Throw for likely option name problems Throw for likely option name problems Jun 13, 2020
@shadowspawn shadowspawn marked this pull request as ready for review June 13, 2020 06:08
@shadowspawn
Copy link
Collaborator Author

Moved out of draft, ready for review.

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.

Thank you for all the great work you do.

@shadowspawn
Copy link
Collaborator Author

Thanks @abetomo

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

Labels

semver: major Releasing requires a major version bump, not backwards compatible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants