Refactoring cli index

[voxsim]

I almost cover with tests every case in src/cli/index.js and after I extracted an help command.

This simplify the file and maybe it enables some other refactorings.

cheers :D
voxsim

EDIT:

Test Plan, I think I covered almost every case in the cli, in detail:

1. run help/--help/-h command
2. run a command with --help/-h option
3. run help/--help/-h with a command name as argument
4. run version/--version command
5. Run install command with no args are provided
6. Run install command if first arg looks like a flag
7. Interpolate aliases properly
8. Yarn should use a flag if provided before --
9. The custom script should use a flag if provided after --

[voxsim]

Ops there are some regressions and I forget to lint my code, sorry ^^'

I'll let you know when I finished ;)

[voxsim]

I finished with one test skipped, but @bestander do you know why we delete the args after -- and some lines after we concatenate for the args that we pass to the command?

References:

• https://github.com/yarnpkg/yarn/blob/master/src/cli/index.js#L32
• https://github.com/yarnpkg/yarn/blob/master/src/cli/index.js#L187

Maybe I can add another test after understanding the use case

Let me know!
cheers,
Simon

[bestander]

@voxsim we ignore arguments after -- for run command, for example.
You can do yarn run test -- no-cache and no-cache argument will be passed through as argument to whatever is executed in test command.

Could you rebase on master to trigger tests?

[bestander]

We moved the hardcoded paths to src/constants.js, could you put it there?

[bestander]

should you have a break statement here?

[bestander]

why commandName is not used anymore?

[voxsim]

This if now covers only the 2nd case, before it was covering the the 1st, 2nd and the 3rd.

[bestander]

@voxsim, thanks for the PR, it looks good.
Could you add Test Plan section to the description of your PR?
We do this to make sure everyone does the minimum testing for changes.

[voxsim]

@bestander I added a Test Plan and rebased with master.

IMHO we should create a Command class to clash the number of ifs and maybe delegate some stuff to it or its child (e.g. only the run command should do stuff with --). If you like the idea I can go ahead with other pull request after this one ;)

[bestander]

@voxsim, thanks for the tests and a great test plan.
I agree, having a single class for all commands flags is hard to maintain.
Go ahead, ping me on the PR!

[TimvdLippe]

It seems that this PR is failing the AppVeyor builds: https://ci.appveyor.com/project/kittens/yarn/build/1646/job/9hcyyn0tj9xh7ldk#L309 This test is also failing for other pull requests.

[bestander]

Thanks, @TimvdLippe.
Appveyor tests were failing for the last few days for another reason so I missed that.
@voxsim, looks like this PR broke on Windows.

should run help of custom-script if --help is after --
    expect(received).toEqual(expected)
    
    Expected value to equal:
      "A message from custom script with args --help"
    Received:
      "Unable to initialize device PRN"
      
      at __tests__\index.js:203:37

[voxsim]

@bestander @TimvdLippe The main problem is the test itself: I do some tricks to test a npm script with arguments and for simplicity I used a bash script. First of all, we can skip the test if we are on windows and maybe think to do another test with the same trick in powershell, what do you think?

[bestander]

Skipping for win32 should be fine imho

…

On Tue, 21 Feb 2017 at 14:13, Simon Vocella ***@***.***> wrote: @bestander <https://github.com/bestander> @TimvdLippe <https://github.com/TimvdLippe> The main problem is the test itself: I do some tricks to test a npm script with arguments and for simplicity I used a bash script. First of all, we can skip the test if we are on windows and maybe think to do another test with the same trick in powershell, what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWGMkHi8JhqMB9BwMZzHXOtmYNgldks5revENgaJpZM4L0fXN> .