Use a class for help command and do some fixes to tests #2810
Conversation
src/util/misc.js
Outdated
| @@ -62,3 +62,13 @@ export function camelCase(str: string): ?string { | |||
| return _camelCase(str); | |||
| } | |||
| } | |||
|
|
|||
| export function findIndex(args: Array<string>, predicate: string): number { | |||
There was a problem hiding this comment.
Why don't you use args.findIndex()?
There was a problem hiding this comment.
Ops you are right ^^' I really don't remember why did this
|
@voxsim, thanks for bringing this up. |
b866205 to
9fd6224
Compare
|
Ping me when it is ready for review |
9fd6224 to
9ba40d2
Compare
|
@bestander I think the code is ready for review; the failure of travis does not seem related to my last changes O.o |
|
I added other commits because I had time and maybe you can give me some feedbacks on how I arrange the cache command class (before it was using buildSubCommand, maybe it is worse than before for you). |
__tests__/index.js
Outdated
|
|
||
| test.concurrent('should run bin command', async () => { | ||
| const stdout = await execCommand('bin', [], '', false); | ||
| expect(stdout[0]).toEqual(`${fixturesLoc}/node_modules/.bin`); |
There was a problem hiding this comment.
this test fails on windows CI (appveyor)
There was a problem hiding this comment.
Oh what a newbie error ^^' i'll fix asap!
c76f977 to
f867ed0
Compare
…; flags and arguments after --
…because the previous one was flaky
f867ed0 to
d6f3e20
Compare
d6f3e20 to
2ae055a
Compare
|
@bestander AppVeyor breaks for both builds for:
That does not seem related to my changes (I haven't changed anything related to the installation command). Could you please check? I'll investigate further in the next days. I added some tests for the cache command, now every change is tested properly! |
|
Maybe this pull request #2887 is better. I will close this after discussing which is better. |
|
Overall I like the BaseCommand/SubCommand idea but I let @arcanis drive this with you |
|
after some parallel work i want to address this in another way ;) tomorrow or sunday I will write something here or write another pull request. Btw do you need some help about addressing issues or bugs? I can help, sorry for the last month but I was busy for personal reasons, but I should come back :p |
|
Yes yes yes! :)
We definitely need help with triagibg bugs and reviewing PRs.
Just go ahead and review PRs and ping me for merges or access to labelling.
We usually give permissions to active members after they've sent a dozen
PRs and shown care for the project.
…On Fri, 7 Apr 2017 at 19:42, Simon Vocella ***@***.***> wrote:
after some parallel work i want to address this in another way ;) tomorrow
or sunday I will write something here or write another pull request.
Btw do you need some help about addressing issues or bugs? I can help,
sorry for the last month but I was busy for personal reasons, but I should
come back :p
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2810 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWOUa2lgkFEqo3POTDhzycgthzSQ7ks5rtoOEgaJpZM4MO7NR>
.
|
|
ok ;) |
|
Right now I am closing this pull request in favor of #3105. Maybe in the future we will extract a base class, we will see :) |
This is my first tiny step towards a Base Command class to clash the number of ifs in src/cli/index.js. The way is quite long :D
Summary
For now I fixed some tests and write the help command as es6 class.
Test plan
I fixed some tests before touching some code; please see the commits.
Added tests for cache command.
@bestander I hope you like what I do and I have some questions for you:
Right I am working to have all classes with three static functions (run, hasWrapper, setFlags), and when I will be ready everything will be switched to instance function. I will open more pull request to be careful ;)