Reduce complexity of src/cli/index.js #2887

voxsim · 2017-03-10T14:08:44Z

After working some time on #2810 I thought that maybe I was trying to reduce the complexity of src/cli/index.js from the wrong point of view.

In this pull request I:

Cherry picked some commits from Use a class for help command and do some fixes to tests #2810
Reduce a lot of if statements
Move all the logic of help in help command

Test plan, add test:

for not camelised command
for not alphabetic command
for not unknown command
extract npm_config_argv_env_vars correctly if you use 'yarn run test' instead of 'yarn test'
display the documentation link correctly if we use an alias
display the documentation link correctly if we use a normal command
do not display the documentation link correctly if we use have an unknown command

Everything else seems tested properly and this change is less invasive than what i do in #2810.

@bestander let me know if you like and sorry if I spam you a lot :P
Feedbacks from everyone are always welcome :)

Questions:

Why we camelcase the command name? And after digging in the function we return null if it has an uppercase letter, otherwise we camelcase the command name. I still don't get what it is the use case O.o

Simon

PS I left some other useful commits from #2810, maybe I will add to this pull request or open others after, in detail:

bestander · 2017-03-14T19:18:02Z

I'll get back to this in a couple of days, thanks for waiting

voxsim · 2017-03-14T23:53:21Z

I added some comments in the code but i will wait your feedback before pushing anything else ;)

voxsim · 2017-03-20T21:13:10Z

I opened other two pull requests for what can be saved from #2810 ;)

voxsim · 2017-04-01T17:15:46Z

@bestander do you have any news on this? :p

bestander · 2017-04-01T17:18:55Z

Sorry, Simon, it is definitely on my list. I'll try to have a look ASAP.

…

On 1 April 2017 at 18:15, Simon Vocella ***@***.***> wrote: @bestander <https://github.com/bestander> do you have any news on this? :p — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2887 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWCXSJ72Tom6hH1NIWQX5L5KX_R62ks5rroZJgaJpZM4MZbH8> .

voxsim · 2017-04-01T17:27:03Z

Hey @bestander , don't worry ;) and thanks for your work!

bestander

Great work, @voxsim!
Let's rebase and merge this.

voxsim · 2017-04-07T14:49:35Z

yeah sure, I need some support from @arcanis for a5a5915. Could you please write me an use case for args = [commandName].concat(getRcArgs(commandName), args);? So I can add a test and see that we won't have any regressions.

@bestander could you please answer about this queston?
Why we camelcase the command name? And after digging in the function we return null if it has an uppercase letter, otherwise we camelcase the command name. I still don't get what it is the use case O.o

I hope to not mistype anything, but sorry I am writing from my phone :p

voxsim · 2017-04-07T15:13:25Z

ok maybe I get it, sorry @arcanis for bother you ;)

bestander · 2017-04-07T15:16:45Z

Why we camelcase the command name? And after digging in the function we return null if it has an uppercase letter, otherwise we camelcase the command name. I still don't get what it is the use case O.o

Just a guess - so that commands could be referred a object properties when needed
like commands.upgradeInteractive.
Although I don't have much background on this issue

voxsim · 2017-04-07T15:21:29Z

mmh if we do that right now It will not work because it has an uppercase letter. If we don't find any use case maybe we should delete that code and let's wait for complains :p

arcanis · 2017-04-07T15:26:37Z

@voxsim you may have already found it, but the code you quoted is related to this PR, and should be covered by these tests 😃 Let me know if you have specific questions about it

(disregard the "test disabled for now" comment, I've just enabled it but forgot to remove the comment)

arcanis · 2017-04-07T15:32:18Z

Btw, I think there's two reasons the command name is camelCased:

It works whether you write yarn foo-bar or yarn fooBar (isn't really useful, but still)
The commands are fetched via an import * as commands from './commands' statement, which means that any command name needs to also be valid a Javascript identifier, otherwise the symbol couldn't be exported from within the commands module. So no dash allowed.

voxsim · 2017-04-07T18:50:41Z

@arcanis i tried both and they work as expected without camelCase. I always advocate "no code over code that i don't understand", but i don't want to be pushy, I'll let decide you, I understand that some time it's better to be conservative ;)

Btw I finished I should push in a bunch of minutes

…; flags and arguments after --

…because the previous one was flaky

…efined

… set commandName as the first arg for npm_config_argv

voxsim · 2017-04-09T14:54:18Z

Ok i finished to rebase everything ;) let me know! For what i see the tests failed for reasons not related to my pull request

voxsim · 2017-04-10T06:29:56Z

I was wrong and after some digging we use the camelCase function only for 'generate-lock-entry' and 'upgrade-interactive'. Maybe we can achieve the same behaviour without camelCase every command. I will prepare another pull request on this part.

arcanis

Nice work, thanks! :) The only thing I'm a bit concerned with (and that's not related to your PR in itself) is that we seem to be hardcoding a lot of logic that should imo be handled by a dedicated third-party library instead. For example, aliases, camelCasing, this cmd hack, the --help output, extracting the command name from the argv, ...

Maybe in a future PR it could be interesting to look if we can use anything out there that would discard the need to maintain all of this logic. What do you think?

arcanis · 2017-04-09T21:52:21Z

src/cli/commands/help.js

+    const commandName = camelCase(args.shift());
+
+    if (commandName) {
+      const command = commands[commandName];


We probably should validate every such access with Object.prototype.hasOwnProperty

If we validate here maybe we should validate in src/cli/index.js. What do you think?

Yep! It will fix this:

❯ [mael-mbp?] yarn git:(0.23-stable) ❯ yarn constructor yarn constructor v0.23.0 error An unexpected error occurred: "command.run is not a function". info If you think this is a bug, please open a bug report with the information provided in "/Users/mael/yarn/yarn-error.log". info Visit https://yarnpkg.com/en/docs/cli/constructor for documentation about this command.

This is a pre-existent error, but i prefer to fix here right now. If I am correctly after this part, we will finish this review.

@arcanis done!

arcanis · 2017-04-09T21:53:03Z

src/cli/commands/help.js

-          continue;
+    const commandName = camelCase(args.shift());
+
+    if (commandName) {


You don't have to check this, since it won't pass the following test (commands[commandName] will be undefined)

I have to check this because commandName can be null, sorry I lost this comment!

Yeah ... it seems this camelCase function should rather be called ifCamelCaseNullElseCamelCase 😄

arcanis · 2017-04-10T09:29:41Z

src/cli/index.js

+let command;
+const camelised = camelCase(commandName);
+if (camelised) {
+  command = commands[camelised];


Why not

commandName = camelCase(commandName); if (!Object.prototype.hasOwnProperty.call(commands, commandName)) { args.unshift(commandName); commandName = `run`; } command = commands[commandName];

?

I added a || commandName because camelCase function return null in some cases.

The final version:

commandName = camelCase(commandName) || commandName; if (!Object.prototype.hasOwnProperty.call(commands, commandName)) { args.unshift(commandName); commandName = 'run'; } command = commands[commandName];

@arcanis I am sorry but I can't do that :( because if you type yarn custom-script it does something horrible because it camel case custom-script.

Oooooh right :/ damn.

arcanis · 2017-04-10T09:32:09Z

src/cli/index.js

+  ...startArgs,
+  // we use this for https://github.com/tj/commander.js/issues/346, otherwise
+  // it will strip some args that match with any options
+  'cmd',


Use something more descriptive than cmd, something like this-arg-will-get-stripped-later. It will help the next person to read this hack to understand how it works, and doesn't risk to conflict with any future command.

arcanis · 2017-04-10T09:36:58Z

src/cli/index.js

-}
-invariant(command, 'missing command');
+// we strip cmd
+commander.args.shift();


We probably want to make a console.assert here to make sure that we're shifting the right argument, what do you think?

Mmh could you please write an example to understand on how you want to use console.assert here?

console.assert(commander.args.length >= 1); console.assert(commander.args[0] === 'cmd'); commander.args.shift();

So that if for some reason we would be shifting the wrong argument, the code early crashes instead of doing something else than what the user expected, which might be hard to debug otherwise.

arcanis · 2017-04-10T09:41:00Z

src/cli/index.js

@@ -382,7 +343,7 @@ config.init({
  httpsProxy: commander.httpsProxy,
  networkConcurrency: commander.networkConcurrency,
  nonInteractive: commander.nonInteractive,
-  commandName,
+  commandName: commandName === 'run' ? commander.args[0] : commandName,


Not sure we can do this; yarn version and yarn run version would both end up with a version commandName, which is semantically wrong because they have different behaviors. If we want to support user-defined commands here, then it should be in a separate field.

Ok this was a pre-existent error, can we open another pull request on this?

Yep, you're right. Let's keep the current behavior

This evening I have to remember to open at least one issue, otherwise I will forget everything :)

voxsim · 2017-03-14T23:47:34Z

src/cli/commands/help.js

-          continue;
+    const commandName = camelCase(args.shift());
+
+    if (commandName) {


I need to check what happened when i type yarn unknown-command --help. It should ignore everything and display standard help. Maybe this behaviour is wrong

OK I was right. In detail: yarn unknown --help

before:

Usage: yarn [command] [flags] Options: -h, --help output usage information -V, --version output the version number --verbose output verbose messages on internal operations --offline trigger an error if any required dependencies are not available in local cache --prefer-offline use network only if dependencies are not available in local cache --strict-semver --json --ignore-scripts don't run lifecycle scripts --har save HAR output of network traffic --ignore-platform ignore platform checks --ignore-engines ignore engines check --ignore-optional ignore optional dependencies --force ignore all caches --no-bin-links don't generate bin links when setting up packages --flat only allow one version of a package --prod, --production [prod] --no-lockfile don't read or generate a lockfile --pure-lockfile don't generate a lockfile --frozen-lockfile don't generate a lockfile and fail if an update is needed --link-duplicates create hardlinks to the repeated modules in node_modules --global-folder <path> --modules-folder <path> rather than installing modules into the node_modules folder relative to the cwd, output them here --cache-folder <path> specify a custom folder to store the yarn cache --mutex <type>[:specifier] use a mutex to ensure only one yarn instance is executing --no-emoji disable emoji in output --proxy <host> --https-proxy <host> --no-progress disable progress bar --network-concurrency <number> maximum number of concurrent network requests

after pr:

Usage: yarn [command] [flags] Options: -h, --help output usage information -V, --version output the version number --verbose output verbose messages on internal operations --offline trigger an error if any required dependencies are not available in local cache --prefer-offline use network only if dependencies are not available in local cache --strict-semver --json --ignore-scripts don't run lifecycle scripts --har save HAR output of network traffic --ignore-platform ignore platform checks --ignore-engines ignore engines check --ignore-optional ignore optional dependencies --force install and build packages even if they were built before, overwrite lockfile --skip-integrity-check run install without checking if node_modules is installed --no-bin-links don't generate bin links when setting up packages --flat only allow one version of a package --prod, --production [prod] --no-lockfile don't read or generate a lockfile --pure-lockfile don't generate a lockfile --frozen-lockfile don't generate a lockfile and fail if an update is needed --link-duplicates create hardlinks to the repeated modules in node_modules --global-folder <path> --modules-folder <path> rather than installing modules into the node_modules folder relative to the cwd, output them here --cache-folder <path> specify a custom folder to store the yarn cache --mutex <type>[:specifier] use a mutex to ensure only one yarn instance is executing --no-emoji disable emoji in output --proxy <host> --https-proxy <host> --no-progress disable progress bar --network-concurrency <number> maximum number of concurrent network requests --non-interactive do not show interactive prompts Commands: - access - add - bin - cache - check - clean - config - generate-lock-entry - global - help - import - info - init - install - licenses - link - list - login - logout - outdated - owner - pack - publish - remove - run - tag - team - unlink - upgrade - upgrade-interactive - version - versions - why Run `yarn help COMMAND` for more information on specific commands. Visit https://yarnpkg.com/en/docs/cli/ to learn more about Yarn.

Good idea! I'd probably put them on a single line, comma-separate, restricted to 80 columns. Otherwise they take a lot of vertical space.

Do you mean the commands part? I can do that if you confirm

I can't overwrite the commands part without doing other horrible things for what I understand because it is a feature of commander.js -.-. In detail the function help.

Actually I think it's done userland, more precisely here. That being said, I've checked locally and it seems that this display (one command each line) was already printed this way in recent Yarn releases, so let's keep it this way. 👍

You are right! As you wish :)

voxsim · 2017-04-10T12:27:08Z

src/cli/index.js

+let command;
+const camelised = camelCase(commandName);
+if (camelised) {
+  command = commands[camelised];


I added a || commandName because camelCase function return null in some cases.

The final version:

commandName = camelCase(commandName) || commandName; if (!Object.prototype.hasOwnProperty.call(commands, commandName)) { args.unshift(commandName); commandName = 'run'; } command = commands[commandName];

voxsim · 2017-04-10T12:30:14Z

src/cli/index.js

@@ -382,7 +343,7 @@ config.init({
  httpsProxy: commander.httpsProxy,
  networkConcurrency: commander.networkConcurrency,
  nonInteractive: commander.nonInteractive,
-  commandName,
+  commandName: commandName === 'run' ? commander.args[0] : commandName,


Ok this was a pre-existent error, can we open another pull request on this?

voxsim · 2017-04-10T12:31:11Z

src/cli/index.js

-}
-invariant(command, 'missing command');
+// we strip cmd
+commander.args.shift();


Mmh could you please write an example to understand on how you want to use console.assert here?

…mmandName and place only a placeholder

voxsim · 2017-04-10T13:07:15Z

Maybe in a future PR it could be interesting to look if we can use anything out there that would discard the need to maintain all of this logic. What do you think?

@arcanis I think you are totally right about that ;)

If you see some something weird about reviews, I am sorry but I forgot to submit many days ago O.o

…s not expected

arcanis · 2017-04-10T16:20:01Z

src/rc.js

@@ -70,7 +70,7 @@ export function getRcArgs(command: string): Array<string> {

  let result = rcArgsCache['*'] || [];

-  if (command !== '*') {
+  if (command !== '*' && Object.prototype.hasOwnProperty.call(rcArgsCache, command)) {


Ah! You got me :)

arcanis

Great job 👍 I'll just wait to see what Travis has to say, but it's good for me

voxsim force-pushed the reduce-complexity-of-tests-index branch 2 times, most recently from 4d8ffe6 to 7272f83 Compare March 10, 2017 16:55

voxsim mentioned this pull request Mar 11, 2017

Use a class for help command and do some fixes to tests #2810

Closed

bestander self-assigned this Mar 14, 2017

bestander self-requested a review March 14, 2017 19:18

bestander removed their assignment Mar 14, 2017

bestander approved these changes Apr 6, 2017

View reviewed changes

bestander requested a review from arcanis April 7, 2017 18:36

voxsim force-pushed the reduce-complexity-of-tests-index branch 2 times, most recently from b231c80 to 0dd467c Compare April 7, 2017 21:02

voxsim added 7 commits April 9, 2017 11:12

clarify initial part of src/cli/index.js when we extracting arguments…

1c3bd30

…; flags and arguments after --

change test if we do not provide any command name and specify a flag …

123b5bf

…because the previous one was flaky

fix silent error in test when we expect an error from a yarn command

6ec7f97

enrich commands with aliases

f268378

simplify if statement: if no command we set install as default

c9affc7

simplify if statement: we always have command with value undefined

04c3ce1

put every logic related to help in help command

89cb665

voxsim added 4 commits April 9, 2017 11:12

use deconstructuring instead of concat and some shift/unshit command

0f268f3

remove useless invariant on commandName: we are sure that is always d…

89fdcbc

…efined

if command is not recognized set default to run; if command is run we…

c227e40

… set commandName as the first arg for npm_config_argv

add some tests cases when we do not recognize command

fbd0c88

voxsim force-pushed the reduce-complexity-of-tests-index branch from 0dd467c to 1e35469 Compare April 9, 2017 09:16

arcanis requested changes Apr 10, 2017

View reviewed changes

voxsim commented Apr 10, 2017

View reviewed changes

voxsim added 4 commits April 10, 2017 15:04

we use commander.js only to parse flags, remove every logic to put co…

3314951

…mmandName and place only a placeholder

implement hasWrapper function for help command

0899ac7

display correct help link for aliases

71072db

display documentation link correctly in every case

4d930e9

voxsim force-pushed the reduce-complexity-of-tests-index branch from 1e35469 to 4d930e9 Compare April 10, 2017 13:08

add some console.asserts to ensure that we early crash if something i…

045bd60

…s not expected

arcanis self-assigned this Apr 10, 2017

fix erronous case: yarn constructor

2fb8a3e

arcanis reviewed Apr 10, 2017

View reviewed changes

arcanis approved these changes Apr 10, 2017

View reviewed changes

arcanis merged commit 45af656 into yarnpkg:master Apr 10, 2017

DanBuild mentioned this pull request Apr 13, 2017

yarn-chocolatey #4 failed #3129

Closed

Reduce complexity of src/cli/index.js #2887

Reduce complexity of src/cli/index.js #2887

Conversation

voxsim commented Mar 10, 2017 • edited Loading

bestander commented Mar 14, 2017

voxsim commented Mar 14, 2017

voxsim commented Mar 20, 2017

voxsim commented Apr 1, 2017

bestander commented Apr 1, 2017 via email

voxsim commented Apr 1, 2017

bestander left a comment

Choose a reason for hiding this comment

voxsim commented Apr 7, 2017 • edited Loading

voxsim commented Apr 7, 2017

bestander commented Apr 7, 2017

voxsim commented Apr 7, 2017

arcanis commented Apr 7, 2017 • edited Loading

arcanis commented Apr 7, 2017 • edited Loading

voxsim commented Apr 7, 2017

voxsim commented Apr 9, 2017

voxsim commented Apr 10, 2017

arcanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxsim Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

voxsim Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxsim Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxsim Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxsim commented Apr 10, 2017 • edited by arcanis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis left a comment

Choose a reason for hiding this comment

voxsim commented Mar 10, 2017 •

edited

Loading

voxsim commented Apr 7, 2017 •

edited

Loading

arcanis commented Apr 7, 2017 •

edited

Loading

arcanis commented Apr 7, 2017 •

edited

Loading

arcanis Apr 10, 2017 •

edited

Loading

voxsim Apr 10, 2017 •

edited

Loading

voxsim Apr 10, 2017 •

edited

Loading

arcanis Apr 10, 2017 •

edited

Loading

voxsim Apr 10, 2017 •

edited

Loading

voxsim Apr 10, 2017 •

edited

Loading

voxsim commented Apr 10, 2017 •

edited by arcanis

Loading