Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Use commander instead of optimist for CLI arguments #2689

Merged
merged 9 commits into from
May 25, 2017

Conversation

andy-hanson
Copy link
Contributor

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

optimist is deprecated. Added a simple options parser that should behave the same as before. (Test with ./bin/tslint.)

@ajafff
Copy link
Contributor

ajafff commented May 5, 2017

I'd prefer using a battle-tested library instead of maintaining our own implementation. There are many alternatives out there: yargs, commander, argparse, ...

@andy-hanson
Copy link
Contributor Author

andy-hanson commented May 6, 2017

Tried using 'commander'.
Found a few issues along the way:
tj/commander.js#629
tj/commander.js#608
tj/commander.js#125
tj/commander.js#482

@nchen63
Copy link
Contributor

nchen63 commented May 6, 2017

why not yargs?

@andy-hanson
Copy link
Contributor Author

Let's install yargs:

└─┬ yargs@8.0.1 
  ├── camelcase@4.1.0 
  ├─┬ cliui@3.2.0 
  │ ├─┬ string-width@1.0.2 
  │ │ ├── code-point-at@1.1.0 
  │ │ ├─┬ is-fullwidth-code-point@1.0.0 
  │ │ │ └── number-is-nan@1.0.1 
  │ │ └── strip-ansi@3.0.1  deduped
  │ ├─┬ strip-ansi@3.0.1 
  │ │ └── ansi-regex@2.1.1 
  │ └─┬ wrap-ansi@2.1.0 
  │   ├─┬ string-width@1.0.2 
  │   │ ├── code-point-at@1.1.0  deduped
  │   │ ├── is-fullwidth-code-point@1.0.0  deduped
  │   │ └── strip-ansi@3.0.1  deduped
  │   └── strip-ansi@3.0.1  deduped
  ├── decamelize@1.2.0 
  ├── get-caller-file@1.0.2 
  ├─┬ os-locale@2.0.0 
  │ ├─┬ execa@0.5.1 
  │ │ ├─┬ cross-spawn@4.0.2 
  │ │ │ ├─┬ lru-cache@4.0.2 
  │ │ │ │ ├── pseudomap@1.0.2 
  │ │ │ │ └── yallist@2.1.2 
  │ │ │ └─┬ which@1.2.14 
  │ │ │   └── isexe@2.0.0 
  │ │ ├─┬ get-stream@2.3.1 
  │ │ │ ├── object-assign@4.1.1 
  │ │ │ └─┬ pinkie-promise@2.0.1 
  │ │ │   └── pinkie@2.0.4 
  │ │ ├── is-stream@1.1.0 
  │ │ ├─┬ npm-run-path@2.0.2 
  │ │ │ └── path-key@2.0.1 
  │ │ ├── p-finally@1.0.0 
  │ │ ├── signal-exit@3.0.2 
  │ │ └── strip-eof@1.0.0 
  │ ├─┬ lcid@1.0.0 
  │ │ └── invert-kv@1.0.0 
  │ └─┬ mem@1.1.0 
  │   └── mimic-fn@1.1.0 
  ├─┬ read-pkg-up@2.0.0 
  │ ├─┬ find-up@2.1.0 
  │ │ └─┬ locate-path@2.0.0 
  │ │   ├─┬ p-locate@2.0.0 
  │ │   │ └── p-limit@1.1.0 
  │ │   └── path-exists@3.0.0 
  │ └─┬ read-pkg@2.0.0 
  │   ├─┬ load-json-file@2.0.0 
  │   │ ├── graceful-fs@4.1.11 
  │   │ ├─┬ parse-json@2.2.0 
  │   │ │ └─┬ error-ex@1.3.1 
  │   │ │   └── is-arrayish@0.2.1 
  │   │ ├── pify@2.3.0 
  │   │ └── strip-bom@3.0.0 
  │   ├─┬ normalize-package-data@2.3.8 
  │   │ ├── hosted-git-info@2.4.2 
  │   │ ├─┬ is-builtin-module@1.0.0 
  │   │ │ └── builtin-modules@1.1.1 
  │   │ ├── semver@5.3.0 
  │   │ └─┬ validate-npm-package-license@3.0.1 
  │   │   ├─┬ spdx-correct@1.0.2 
  │   │   │ └── spdx-license-ids@1.2.2 
  │   │   └── spdx-expression-parse@1.0.4 
  │   └─┬ path-type@2.0.0 
  │     └── pify@2.3.0  deduped
  ├── require-directory@2.1.1 
  ├── require-main-filename@1.0.1 
  ├── set-blocking@2.0.0 
  ├─┬ string-width@2.0.0 
  │ ├── is-fullwidth-code-point@2.0.0 
  │ └── strip-ansi@3.0.1  deduped
  ├── which-module@2.0.0 
  ├── y18n@3.2.1 
  └─┬ yargs-parser@7.0.0 
    └── camelcase@4.1.0  deduped
$ wc `find -name '*.js'` --lines
...
13581 total

That's a pretty big install for something that only took me 40 lines of code.

Now let's install commander:

└─┬ commander@2.9.0 
  └── graceful-readlink@1.0.1
wc `find -name '*.js'` --lines
   12 ./node_modules/graceful-readlink/index.js
 1110 ./node_modules/commander/index.js
 1122 total

I know commander had a few bugs, but yargs is bound to have proportionately more.

@nchen63
Copy link
Contributor

nchen63 commented May 12, 2017

commander doesn't seem to be very active. The last commit was 3 months ago

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commander seems fine; it's at least better than the deprecated optimist

overall these changes look good, but need to resolve some conflicts

@adidahiya adidahiya changed the title Remove "optimist" dependency Use commander instead of optimist for CLI arguments May 17, 2017
@adidahiya
Copy link
Contributor

package.json / yarn.lock conflicts

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

Successfully merging this pull request may close these issues.

4 participants