Skip to content

fix(defaults): Process help and version defaults better#116

Merged
ntwcklng merged 7 commits intoleo:masterfrom
markhalliwell:version-help-fix
Apr 3, 2018
Merged

fix(defaults): Process help and version defaults better#116
ntwcklng merged 7 commits intoleo:masterfrom
markhalliwell:version-help-fix

Conversation

@markhalliwell
Copy link
Copy Markdown
Contributor

@markhalliwell markhalliwell commented Jan 2, 2018

The predefined help and version options are hardcoded to the lowercase characters when checking if they were provided.

This causes issues if other options like host or verbose are provided first, which actually does cause the help and version short options to be uppercased, but are ultimately ignored.

Summary of changes:

Fixes

  • Hard coded help/version short options. If someone provides -v for verbose, they actually get the version printed out and the process exits (this may also possibly help with subcommands can't have a --version option #103 somewhat):
    Before: foo bar -v -> 1.0.0
    After: foo bar -v -> Verbose output from bar
  • Options passed before command:
    Before: foo -dv bar -f file.txt -> foo-bar -f file.txt
    After: foo -dv bar -f file.txt -> foo-bar -dv -f file.txt
  • Bug in generateDetails where it was overriding the usage property on the items it was generating, which in turn was also altering the original object and causing the tests to bork.
  • Updated documentation

Added

Removed

  • The parent parameter that was added to checkVersion in pass in parent module from index.js #96. Since this was only introduced in 3.0.2 and has remained an internal method (not exposed until now), there's no real risk of breaking BC.
  • The pkginfo dependency - This module is unnecessary and, quite frankly, a little too presumptuous in populating metadata onto the module object. While, perhaps useful for other projects, it is overkill for simply attempting to retrieve the main module's version.

@ntwcklng
Copy link
Copy Markdown
Collaborator

ntwcklng commented Jan 2, 2018

Awesome PR! Would you mind and add tests for both cases like host and verbose?

@markhalliwell
Copy link
Copy Markdown
Contributor Author

markhalliwell commented Jan 2, 2018

Ok, I've gone ahead and added separate tests (which really need some love and to be separated even more BTW).

I ended up having to implement an exit config option so we can test the actual flags without the process exiting (which pretty much fixes #95).

Found a nifty little bug in generateDetails where it was overriding the usage property on the items it was generating, which in turn was also altering the original object and causing the tests to bork.

Removed the parent parameter that was added to checkVersion in #96. Since this was only introduced in 3.0.2 and has remained an internal method (not exposed until now), there's no real risk of breaking BC.

Updated the docs as well.

@markhalliwell
Copy link
Copy Markdown
Contributor Author

Seems Travis is having some network issues. Any chance the last commit test can be restarted?

@markhalliwell
Copy link
Copy Markdown
Contributor Author

Cleaned up the version code a bit so it uses process.mainModule instead of module.parent.

I also went ahead and provided a summary of changes for the PR.

I think I'm done tinkering with this, for now, don't want to scope creep too much.

@ntwcklng ntwcklng requested a review from leo January 5, 2018 15:49
@ntwcklng ntwcklng mentioned this pull request Apr 3, 2018
@ntwcklng ntwcklng merged commit 19fecc4 into leo:master Apr 3, 2018
@markhalliwell markhalliwell deleted the version-help-fix branch April 3, 2018 20:07
joshbetz added a commit to Automattic/vip-cli that referenced this pull request Jun 21, 2018
joshbetz added a commit to Automattic/vip-cli that referenced this pull request Jun 22, 2018
leo pushed a commit that referenced this pull request May 1, 2022
* fix(defaults): Process help and version defaults better

Also removes unnecessary "pkginfo" dependency

* fix(command): Pass any options that appear before a command

* task(version): Remove unnecessary parent parameter introduced in #96

feat(config): Add "exit" booleans for help and version

fix(help): Clone objects in array so original data is not altered

Fixes #95

* test(options): Add assertions to ensure "help" and "version" options work properly

* docs(options): Add ".showVersion()" method and "exit" configuration

* fix(generateDetails): Use Object.assign instead of spread for older node versions

* fix(version): Use process.mainModule instead of module.parent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants