Add include to CLI options
#214
Conversation
| @@ -46,6 +46,11 @@ var yargs = require('yargs/yargs')(process.argv.slice(2)) | |||
| type: 'boolean', | |||
| describe: 'whether or not to instrument all files of the project (not just the ones touched by your test suite)' | |||
| }) | |||
| .option('include', { | |||
| alias: 'in', | |||
There was a problem hiding this comment.
i is taken unfortunately.
|
Also not 100% sure I did everything right, but I tested it in my project and it works fine. |
|
If we add |
| @@ -46,6 +46,11 @@ var yargs = require('yargs/yargs')(process.argv.slice(2)) | |||
| type: 'boolean', | |||
| describe: 'whether or not to instrument all files of the project (not just the ones touched by your test suite)' | |||
| }) | |||
| .option('include', { | |||
| alias: 'in', | |||
| default: [], | |||
There was a problem hiding this comment.
Also from readme:
Note: include defaults to ['**']
But this default value probably defined somewhere else in the code. Should we remove it there, and specify ['**'] here. Although we need default value when it's loaded from package.json too, so maybe only specify here and not remove in other place?
There was a problem hiding this comment.
The code arrives at that ** default implicitly. I think we'd need a guard here for empty config.include arrays.
|
I haven't check yet that the latest changes work. Will check in my project a bit later. |
|
Checked:
Looking into the last case. |
|
The last commit fixes last case. |
|
@rpominov I really like this work 👍 would it be possible to add add a test; no need to add one for the CLI (since we don't have any tests for this yet, although it would be great to have some eventually). But it would be nice to have tests for the new logic that appends a list of excludes provided to the default excludes, and logic for the behavior around an include that's an empty list. Also, mind rebasing to resolve conflicts 👍 |
|
@bcoe Ok, no problem. I think I'll be able to do this at the end of the week. |
rpominov
force-pushed
the
include-option-cli
branch
from
0e42598 to
5f439a8
Compare
|
I've rebased and added the tests, hope they're ok. Also I wasn't able to make all current tests pass on my machine (in master, without changes of this PR). In particular this assertion fails, and this code fails with "file already exists" error. Probably this is just something wrong with my setup... Also, man, running those tests splitted to individual files takes forever 😯 |
|
LGTMO! |
|
|
||
| var nyc = (new NYC({ | ||
| require: argv.require | ||
| require: argv.require, | ||
| include: argv.include, |
There was a problem hiding this comment.
@novemberborn Maybe we could check for ** here before passing to new NYC?
There was a problem hiding this comment.
Yea, or we change the NYC implementation. Let's see what @bcoe suggests.
|
@novemberborn @rpominov I made a couple small tweaks, and added tests. see: |
some small tweaks to --include, --exclude also adds tests
|
@bcoe Thanks, merged. The commits log of this PR gets a little messy, should I squash the commits? |
|
@rpominov no worries 👍 I've started using the squash button when I merge commits, along with this slick new library: https://github.com/conventional-changelog/standard-version |
|
Ah, right, forgot that github now can do squash. And standard-version looks very cool 👍 |
|
@rpominov this is now released in |
|
@bcoe Great. Thanks! |
See #207