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

Throw for likely option name problems #1275

Merged
merged 7 commits into from Jun 15, 2020

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

@shadowspawn shadowspawn merged commit 42f61ca into tj:release/6.x Jun 15, 2020
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 15, 2020
@shadowspawn shadowspawn added this to the v6.0.0 milestone Jun 20, 2020
@shadowspawn shadowspawn deleted the feature/warn-clashes branch July 18, 2020 22:44
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jul 19, 2020
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.

None yet

2 participants