Fix: fix incorrect argument parsing #4384

BYK · 2017-09-10T17:54:28Z

Summary

Fixes #4383. This patch makes argument parsing a bit tidier, and
starts supporting yarn --silent custom-script style commands as
initiall intended by #4152.

Test plan

Existing unit tests. Should ideally add a few more.

**Summary** Fixes #4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by #4152. **Test plan** Existing unit tests.

buildsize · 2017-09-10T19:15:27Z

This change will increase the build size from 9.51 MB to 9.52 MB, an increase of 2.01 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	819.63 KB	819.8 KB	181 bytes (0%)
yarn-[version].js	3.62 MB	3.62 MB	668 bytes (0%)
yarn-legacy-[version].js	3.67 MB	3.67 MB	801 bytes (0%)
yarn-v[version].tar.gz	823.76 KB	823.96 KB	207 bytes (0%)
yarn_[version]all.deb	626.48 KB	626.68 KB	206 bytes (0%)

Daniel15 · 2017-09-11T18:38:32Z

__tests__/fixtures/index/run-custom-script-with-arguments/package.json

@@ -3,6 +3,6 @@
  "version": "1.0.0",
  "license": "UNLICENSED",
  "scripts": {
-    "custom-script": "print() { echo \"A message from custom script with args \"$@\"\"; }; print "
+    "custom-script": "node echo.js"


Could use

"custom-script": "node -e 'console.log(`A message from custom script with args ${process.argv[process.argv.length - 1]}`)'"

To avoid the need to have a separate .js file.

That's what I did at first (I actually used -p) but Node 4 rejects additional arguments after the passed in script block. I used -- to make it ignore but this was only added in Node 7 or 6 so it failed on Node 4 builds, hence this current, "ugly" solution :(

Daniel15 · 2017-09-11T18:38:58Z

src/cli/index.js

-  if (commandName === '--help' || commandName === '-h') {
-    commandName = 'help';
+  // get command name
+  const firstNonFlagIndex = args.findIndex(arg => !arg.startsWith('-'));


Are there unit tests covering all this stuff?

BYK · 2017-09-11T18:46:10Z

@arcanis give your final blessing and we can merge!

arcanis

This commit does a lot of things - I'm fine with it, but I think the next one we do on the CLI will have to be about switching to another CLI library, because otherwise it will become even harder to make the call later on.

arcanis · 2017-09-11T15:45:53Z

__tests__/fixtures/index/run-custom-script-with-arguments/echo.js

@@ -0,0 +1 @@
+console.log(`A message from custom script with args ${process.argv[process.argv.length - 1]}`);


Have you considered writing JSON instead and using .toMatchObject?

Can you elaborate? I haven't considered that and don't understand what exactly you're proposing but it sounds interesting :)

You could

console.log(JSON.stringify(process.argv))

Then in the test:

expect(JSON.parse(stdout)).toEqual([ '--help' ]);

I think it would be semantically more sound because we wouldn't test the rest of the message on top of the args (A message from custom script etc).

Like it. Done!

arcanis · 2017-09-11T15:47:34Z

src/cli/index.js

-  if (commandName === '--help' || commandName === '-h') {
-    commandName = 'help';
+  // get command name
+  const firstNonFlagIndex = args.findIndex(arg => !arg.startsWith('-'));


The problem with this approach is that it falls flat with options that expect arguments (like --mutex) :(

Have you tried running yarn --mutex network? If I'm reading this correctly, firstNonFlagIndex will point to network.

Dang, good catch! Will fix this.

BYK

This commit does a lot of things

Ehh. I mean I tried to fix it in a simpler way which broke a bunch of other things so I had to do this larger change.

I'm fine with it, but I think the next one we do on the CLI will have to be about switching to another CLI library, because otherwise it will become even harder to make the call later on.

Exactly what I was thinking. I actually decided to write a yet another argument parser (not being sarcastic!) after dealing with this craziness 😁

BYK · 2017-09-12T09:49:52Z

__tests__/fixtures/index/run-custom-script-with-arguments/echo.js

@@ -0,0 +1 @@
+console.log(`A message from custom script with args ${process.argv[process.argv.length - 1]}`);


Can you elaborate? I haven't considered that and don't understand what exactly you're proposing but it sounds interesting :)

BYK · 2017-09-12T09:50:18Z

src/cli/index.js

-  if (commandName === '--help' || commandName === '-h') {
-    commandName = 'help';
+  // get command name
+  const firstNonFlagIndex = args.findIndex(arg => !arg.startsWith('-'));


Dang, good catch! Will fix this.

arcanis · 2017-09-12T10:42:56Z

Exactly what I was thinking. I actually decided to write a yet another argument parser (not being sarcastic!) after dealing with this craziness 😁

Why not improve concierge instead? There's even (a few) tests! :D

**Summary** Fixes yarnpkg#4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by yarnpkg#4152. **Test plan** Existing unit tests. Should ideally add a few more.

Fix: fix incorrect argument parsing

5feff12

**Summary** Fixes #4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by #4152. **Test plan** Existing unit tests.

BYK assigned arcanis Sep 10, 2017

BYK requested a review from arcanis September 10, 2017 17:54

BYK added 2 commits September 10, 2017 21:32

Fix bug around help detection

dba540e

bash vs cmd

bd8bead

BYK added 3 commits September 11, 2017 15:03

Merge branch 'master' into sane-parsing

bb16603

Make script test Node 4 compatible

7d28dfa

Add forgotten file

8b26be1

Daniel15 approved these changes Sep 11, 2017

View reviewed changes

arcanis reviewed Sep 12, 2017

View reviewed changes

BYK commented Sep 12, 2017

View reviewed changes

BYK added 2 commits September 12, 2017 10:58

Merge branch 'master' into sane-parsing

5624d52

Handle options taking arguments

5a18bf7

BYK added 2 commits September 12, 2017 12:08

Use JSON parsing for tests

8ac2e1d

Why, thank you flow

8a4e371

arcanis approved these changes Sep 12, 2017

View reviewed changes

BYK merged commit cb3c7fe into master Sep 12, 2017

BYK deleted the sane-parsing branch September 12, 2017 12:41

BYK added this to the 1.0.2 milestone Sep 12, 2017

BYK mentioned this pull request Sep 15, 2017

Confusing order of CLI flags #4470

Open

		@@ -0,0 +1 @@
		console.log(`A message from custom script with args ${process.argv[process.argv.length - 1]}`);

Fix: fix incorrect argument parsing #4384

Fix: fix incorrect argument parsing #4384

Uh oh!

Conversation

BYK commented Sep 10, 2017

Uh oh!

buildsize bot commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK commented Sep 11, 2017

Uh oh!

arcanis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis commented Sep 12, 2017

Uh oh!

Uh oh!

buildsize bot commented Sep 10, 2017 •

edited

Loading

arcanis Sep 11, 2017 •

edited

Loading