Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed yargs default for reporters so the ones defined in are taken first #185

Merged
merged 1 commit into from
Mar 12, 2016

Conversation

rapzo
Copy link
Contributor

@rapzo rapzo commented Mar 6, 2016

As pointed in issue #184.

@bcoe
Copy link
Member

bcoe commented Mar 6, 2016

wouldn't this approach make it so that it's impossible to override the reporter:

nyc --reporter=lcov-text

the goal is a solution that uses the reporter defined in package.json iff the default reporter has been set by yargs?

@rapzo
Copy link
Contributor Author

rapzo commented Mar 6, 2016

Yeah, you're right...
Well, i thought the expected behaviour was to run through the list of reporters defined in package.json when no reporter was provided by yargs. Shouldn't it be?

@rapzo
Copy link
Contributor Author

rapzo commented Mar 6, 2016

Went a bit up and removed the yargs' default for reporters since it is set here when none provided. Seems to work both ways.

@bcoe
Copy link
Member

bcoe commented Mar 8, 2016

@rapzo I dug into this a bit more, there's a slightly better solution that allows us to continue having a default value:

  .option('reporter', {
    alias: 'r',
    describe: 'coverage reporter(s) to use',
    global: true
  })

there appears to be a bug with yargs, which is taking into account aliases when loading defaults from the nyc stanza in the package.json. A temporary fix is to edit bin/nyc.js so that the long option name is used for the option rather than the alias.

I'd happily land this patch, but we should also dig into this bug in yargs.

@rapzo
Copy link
Contributor Author

rapzo commented Mar 10, 2016

Sorry for the delay. Here's the patch. A better solution indeed.

bcoe added a commit that referenced this pull request Mar 12, 2016
Removed yargs default for reporters so the ones defined in  are taken first
@bcoe bcoe merged commit 85a9a46 into istanbuljs:master Mar 12, 2016
@bcoe
Copy link
Member

bcoe commented Mar 13, 2016

@rapzo give this a shot, shipped with your fixes for package configuration:

npm i nyc@next

If everything is looking good I'll promote the change to latest soon.

@rapzo
Copy link
Contributor Author

rapzo commented Mar 13, 2016

I will! Thanks man!

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.

2 participants