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

Add a new argument to specify a different path for nycrc #724

Conversation

igorlima
Copy link
Contributor

@igorlima igorlima commented Nov 17, 2017

This PR allows us to specify a different path for .nycrc.

NOTE: Some projects need different settings for backend and frontend tests. I'm working on a project that we do need different settings for both backend and frontend tests.

Also mocha allows us to do it as well, by providing the argument --opts.

Thanks for understanding.

@pedrocelso
Copy link

LGTM

1 similar comment
@tbonfim
Copy link

tbonfim commented Nov 17, 2017

LGTM

@JaKXz JaKXz requested a review from bcoe November 17, 2017 19:18
@@ -40,7 +40,11 @@ function loadConfig (argv, cwd) {
// that would cause the application to exit early.
Config.buildYargs = function (cwd) {
cwd = guessCWD(cwd)
const config = loadConfig()
const argv = Yargs(process.argv)
.help(false)
Copy link
Member

Choose a reason for hiding this comment

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

I think we could avoid needing to call Yargs(process.argv) here if we delay calling this line:

https://github.com/igorlima/nyc/blob/486aacc1ba1702c5f10505664cdefaa7368703ef/lib/config-util.js#L220

I think we could then swap out the config path in the nyc.js bin:

https://github.com/igorlima/nyc/blob/486aacc1ba1702c5f10505664cdefaa7368703ef/bin/nyc.js#L21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcoe thanks for the quick feedback.

Done. Addressed already at b01a3f3.

  • avoid needing to call Yargs(process.argv)
  • swap out the config path in the bin/nyc.js

Let me know if there's any other concern.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

one comment about avoiding running Yargs() in config-util.js. This work is looking great 👍 thank you.

- avoid needing to call Yargs(process.argv)
- swap out the config path in the nyc.js bin
@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Changes Unknown when pulling b01a3f3 on igorlima:add-a-new-argument-to-specify-different-path-for-nycrc into ** on istanbuljs:master**.

@coveralls
Copy link

coveralls commented Nov 21, 2017

Coverage Status

Changes Unknown when pulling 4895ae3 on igorlima:add-a-new-argument-to-specify-different-path-for-nycrc into ** on istanbuljs:master**.

@igorlima
Copy link
Contributor Author

@bcoe let me know if there's any other concern. Thanks.

@bcoe bcoe merged commit 785fccb into istanbuljs:master Nov 27, 2017
@bcoe
Copy link
Member

bcoe commented Nov 27, 2017

@igorlima thank you for the contribution 😄

@astorije
Copy link

Awesome stuff, @igorlima! @bcoe, when can we expect it in a release? :)) Thanks all!

@bcoe
Copy link
Member

bcoe commented Nov 28, 2017

@astorije @igorlima please try this:

npm i nyc@next version 11.4.0 is in the queue, and will be the next release.

@astorije
Copy link

I'm using the master version right now, but will switch to @next until 11.4.0 gets out! Thanks!

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.

6 participants