fix(cli): improve handling of forwarding args/flags #101

kentcdodds · 2017-02-09T22:40:49Z

What: This migrates us from commander and omelette to yargs
and in the process changes how we handle forwarding of arguments and
flags. We no longer just arbitrarily forward arguments to every script.
Also instead of comma separated scripts, they're simply separated by a
space. If you want to pass args/flags to a script, then you put it in
quotes.

Why: Makes this more predictable. Also yargs is more powerful in
general anyway.

How: Magic... Will comment inline...

Closes #70 and #100

kentcdodds · 2017-02-10T00:19:01Z

Anyone watching the repo can probably tell that I'm at a total loss on why the build is failing. Anyone wanna give it a look and figure out what's up? I think somehow that p-s is spawning processes in an old version of node... Or something. Totally bonkers.

kentcdodds · 2017-02-10T03:25:30Z

I'm fairly confident that I've narrowed it down to spawn-default-shell (dependency of concurrently) being the problem (cc @kimmobrunfeldt). My guess is that the way that it determines the shell to use ignores nvm which Travis has set up for the correct node version and it uses the pre-existing node version. I'm working on a repro repo right now.

kentcdodds · 2017-02-10T03:46:16Z

Verified

kentcdodds · 2017-02-10T03:58:27Z

Looks like my fork fixes the issue. Just waiting on this to be merged and released or finding another solution.

codecov-io · 2017-02-12T18:25:43Z

Codecov Report

Merging #101 into next will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           next   #101   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        14     10    -4     
  Lines       246    286   +40     
===================================
+ Hits        246    286   +40

Impacted Files	Coverage Δ
src/bin-utils/index.js	`100% <ø> (ø)`	✅
src/bin-utils/parser.js	`100% <100%> (ø)`
src/get-logger.js	`100% <100%> (ø)`	✅
src/bin-utils/autocomplete-get-scripts.js	`100% <100%> (ø)`
src/index.js	`100% <100%> (ø)`	✅
src/bin-utils/fixtures/fake-es6-module.js
src/bin-utils/fixtures/function-config.js
src/bin-utils/fixtures/my-module.js
src/bin-utils/fixtures/bad-data-type-config.js
...in-utils/fixtures/bad-function-data-type-config.js
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c04785...67dbed7. Read the comment docs.

kentcdodds · 2017-02-12T18:32:40Z

Wahoo! This PR is ready for review finally!

kentcdodds · 2017-02-12T18:49:41Z

cli-test/cli.test.js

@@ -4,11 +4,6 @@ import runNPS from './run-nps'
 const fixturesPath = resolve(__dirname, './fixtures')

 test(
-  'without arguments',


I removed this because yargs help output is non-deterministic and based on the width of the terminal.

kentcdodds · 2017-02-12T19:06:27Z

src/index.test.js

@@ -60,24 +60,15 @@ test('options: logLevel sets the log level', () => {
  })
 })

-test('passes on additional arguments', () => {


We don't worry about this anymore. This is no longer a feature. If you want to pass arguments, put it in quotes. It's less confusing this way

addityasingh · 2017-02-13T11:17:56Z

@kentcdodds The changes look fine, but do you think it would look a clean syntax?

nps 'build --foo'

My 2 cents: I would still prefer to not have any special syntax for my cli commands and below would be better than above:

nps build --foo

kentcdodds · 2017-02-13T13:56:07Z

Thanks for the review @addityasingh (I've added you to the contributors table as a reviewer!),
I'll respond to you on #100

addityasingh · 2017-02-13T22:52:41Z

README.md

+  -h, --help       Show help                                           [boolean]
+  -v, --version    Show version number                                 [boolean]
+
+Examples:


Examples really shine through 👍

addityasingh · 2017-02-13T22:53:50Z

README.md

-##### scripts
-
-To run a script, you simply provide the name of the script like so:
+And you can run multiple scripts in series by simply adding more arguments.


Maybe change wording to: And you can run multiple scripts in series by simply adding more space-separated arguments.?

addityasingh · 2017-02-13T23:05:18Z

src/get-logger.js

@@ -50,6 +52,16 @@ function getShouldLogFn(...acceptableValues) {
  acceptableValues = ['', 'debug', ...acceptableValues]


I think it's preferable to treat params (acceptableValues ) as const and not to override them, to avoid any kind of confusion in readability

addityasingh · 2017-02-13T23:07:25Z

src/get-logger.js

+  }
+}
+
+function getLogLevel({silent, logLevel}) {


This whole function can be refactored as below:

const getLogLevel = ({silent, logLevel}) => silent ? 'disable' : logLevel

**What**: This migrates us from `commander` and `omelette` to `yargs` and in the process changes how we handle forwarding of arguments and flags. We no longer just arbitrarily forward arguments to every script. Also instead of comma separated scripts, they're simply separated by a space. If you want to pass args/flags to a script, then you put it in quotes. **Why**: Makes this more predicatble. Also `yargs` is more powerful in general anyway. **How**: Magic... Will comment inline...

kentcdodds · 2017-02-14T00:41:43Z

I'm going to merge this now and publish a beta for the next branch. I think there's still more to do, but I want to get feedback!

**What**: This migrates us from `commander` and `omelette` to `yargs` and in the process changes how we handle forwarding of arguments and flags. We no longer just arbitrarily forward arguments to every script. Also instead of comma separated scripts, they're simply separated by a space. If you want to pass args/flags to a script, then you put it in quotes. **Why**: Makes this more predicatble. Also `yargs` is more powerful in general anyway. **How**: Magic... Will comment inline...

gunnx · 2017-02-22T09:02:00Z

I just updated and realised I can no longer pass down args as I was doing before:
e.g.
npm start lint - run linter
npm start lint -- --fix - run linter with fix flag passed

I guess this was just useful sometimes without having to create every possible option in package-scripts

kentcdodds · 2017-02-22T14:09:05Z

As noted in the release notes

Another significant change here is the change for #100.

before:
nps lint,build "src/**/*.js"
after:
nps "lint src/**/*.js" "build src/**/*.js"
It's kinda sad, but I honestly never really saw anyone utilizing the argument forwarding feature anyway. Where this gets a little more impactful is when you're typing at the command line:

before:
nps test --updateSnapshot
after:
nps "test --updateSnapshot"
Notice the quotes. Those are now required for a command and its arguments to be grouped (otherwise --updateSnapshot will be assumed to be a command you're trying to execute and you'll get an error). Actually, nps will throw an error if you try nps foo --flag because --flag is not a recognized nps flag :)

:)

kentcdodds requested review from nkbt, rowanoulton, osdevisnot, martellaj, sxn, jisaacks, giladgo, gunnx and tleunen February 9, 2017 22:40

kentcdodds force-pushed the pr/yargs branch 6 times, most recently from 10d6096 to 0ad7bba Compare February 10, 2017 00:15

kentcdodds mentioned this pull request Feb 10, 2017

spawns with a different version of node on Travis kimmobrunfeldt/spawn-default-shell#8

Closed

kentcdodds force-pushed the pr/yargs branch from 42e2d51 to 0ad7bba Compare February 10, 2017 03:46

kentcdodds commented Feb 12, 2017

View reviewed changes

kentcdodds force-pushed the pr/yargs branch from 96e18ea to faf5b66 Compare February 13, 2017 13:55

addityasingh reviewed Feb 13, 2017

View reviewed changes

kentcdodds force-pushed the pr/yargs branch from faf5b66 to 67dbed7 Compare February 14, 2017 00:36

kentcdodds merged commit a15d56a into next Feb 14, 2017

kentcdodds deleted the pr/yargs branch February 14, 2017 00:41

dweber019 mentioned this pull request Apr 16, 2017

How can I pass arguments to an nps command? #133

Open

kentcdodds mentioned this pull request May 17, 2017

Revert 100 #139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): improve handling of forwarding args/flags #101

fix(cli): improve handling of forwarding args/flags #101

kentcdodds commented Feb 9, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

codecov-io commented Feb 12, 2017 •

edited

Loading

kentcdodds commented Feb 12, 2017

kentcdodds Feb 12, 2017

kentcdodds Feb 12, 2017

addityasingh commented Feb 13, 2017

kentcdodds commented Feb 13, 2017

addityasingh Feb 13, 2017

addityasingh Feb 13, 2017

addityasingh Feb 13, 2017

addityasingh Feb 13, 2017

kentcdodds commented Feb 14, 2017

gunnx commented Feb 22, 2017

kentcdodds commented Feb 22, 2017

		@@ -50,6 +52,16 @@ function getShouldLogFn(...acceptableValues) {
		acceptableValues = ['', 'debug', ...acceptableValues]

fix(cli): improve handling of forwarding args/flags #101

fix(cli): improve handling of forwarding args/flags #101

Conversation

kentcdodds commented Feb 9, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

kentcdodds commented Feb 10, 2017

codecov-io commented Feb 12, 2017 • edited Loading

Codecov Report

kentcdodds commented Feb 12, 2017

kentcdodds Feb 12, 2017

Choose a reason for hiding this comment

kentcdodds Feb 12, 2017

Choose a reason for hiding this comment

addityasingh commented Feb 13, 2017

kentcdodds commented Feb 13, 2017

addityasingh Feb 13, 2017

Choose a reason for hiding this comment

addityasingh Feb 13, 2017

Choose a reason for hiding this comment

addityasingh Feb 13, 2017

Choose a reason for hiding this comment

addityasingh Feb 13, 2017

Choose a reason for hiding this comment

kentcdodds commented Feb 14, 2017

gunnx commented Feb 22, 2017

kentcdodds commented Feb 22, 2017

codecov-io commented Feb 12, 2017 •

edited

Loading