Skip to content

Conversation

@kodie
Copy link
Contributor

@kodie kodie commented Aug 22, 2017

Solves Issue #26.

@coveralls
Copy link

coveralls commented Aug 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2f65be7 on kodie:master into 359aa4b on adamreisnz:master.

Copy link
Owner

@adamreisnz adamreisnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kodie, thanks for the PR! Could you please address the two comments? Rest looks good!


//Get from/to parameters from CLI args if not defined in config file
if (typeof from === 'undefined') {
from = argv._.shift();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to use shift here because we can't be sure of the index. For example, from might be defined in the config already, and then it's not shifted, but then to will be in position 0.

bin/cli.js Outdated
//Get files
if (!files) {
files = argv._;
files = argv._[2];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a breaking change, people might rely on this functionality.
I would prefer to not pass the ignore files via the CLI but rather use the config file, or a named parameter, e.g. --ignore

@kodie
Copy link
Contributor Author

kodie commented Aug 24, 2017

@adamreisnz How's that?

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 75ffb25 on kodie:master into 359aa4b on adamreisnz:master.

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 628492d on kodie:master into 359aa4b on adamreisnz:master.

Copy link
Owner

@adamreisnz adamreisnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! I'll merge it in shortly.

@adamreisnz adamreisnz merged commit 508ad44 into adamreisnz:master Aug 24, 2017
@adamreisnz adamreisnz mentioned this pull request Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants