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

all of nyc's configuration options can now also be set in package.json #167

Merged
merged 2 commits into from
Feb 20, 2016

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Feb 14, 2016

This will need to be a major bump, because I've retired config.nyc.

This upgrades us to yargs 4.x which is a pretty major change (so that I can use the new pkgConf feature), this is a pretty major change and I would love for folks to take an extra close look to make sure I haven't broken anything.

Note that I could delete a bunch of code with yargs' improved command functionality \o/

Reviewers: @novemberborn, @jamestalmage, @jameswomack, @lloydcotten

@lloydcotten
Copy link
Contributor

I took a look through this, looking mostly at how it might affect the extensions feature. I think I like the changes. I did try it out on on of my projects, but I'm getting an error from istanbul, I think that was introduced in this recent merge 9fb7da4 from @novemberborn

I'll try to drill down to see what's causing it more specifically and comment over on that PR.

@bcoe
Copy link
Member Author

bcoe commented Feb 15, 2016

@lloydcotten if you delete node_modules/.cache does it still crash?

I was seeing a similar problem, we should get to the bottom of this before we publish.

@lloydcotten
Copy link
Contributor

Hmm... you are right. That does resolve it. I was seeing this:

/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:288
                    text = structuredText[startLine].text;
                                                    ^

TypeError: Cannot read property 'text' of undefined
    at /home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:288:53
    at Array.forEach (native)
    at annotateBranches (/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:255:30)
    at HtmlReport.Report.mix.writeDetailPage (/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:426:9)
    at /home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:489:26
    at SyncFileWriter.extend.writeFile (/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/util/file-writer.js:57:9)
    at FileWriter.extend.writeFile (/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/util/file-writer.js:147:23)
    at /home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:488:24
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeFiles (/home/lloyd/dev/github/nyc/node_modules/istanbul/lib/report/html.js:482:23)

@bcoe
Copy link
Member Author

bcoe commented Feb 15, 2016

@lloydcotten do you caching turned on?

@lloydcotten
Copy link
Contributor

Yes, I had caching on.

@novemberborn
Copy link
Contributor

@bcoe @lloydcotten the cache uses a salt derived from NYC's and Istanbul's version numbers. The version hasn't yet changed from before my path changes (it's still 5.5.0) so it may have used something that was actually too old. The risk of using code from GitHub branches I suppose.

Remove tests for config.nyc entries in package.json. Using the fixture
package.json for testing. Clean up config in actual package.json that was there
for test purposes only.
@novemberborn
Copy link
Contributor

@bcoe looks good!

I took the liberty of pushing a commit which fixes some duplicate config tests. We're now also using fixtures/package.json to test config, allowing me to remove .es6 extension from the actual package.json.

@bcoe
Copy link
Member Author

bcoe commented Feb 15, 2016

@novemberborn from your discussion with @lloydcotten, it sounds like the act of bumping the version will fix the cache issue?

I'll make this [email protected], since it changes config's functionality quite a bit.

@novemberborn
Copy link
Contributor

@bcoe I'm not sure why cache entries from an older version don't work, but if clearing the cache fixes the issue, then yes bumping the version should preclude any older cache entries from being used.

var config = this._loadConfig(opts)
this._istanbul = config.istanbul
this.subprocessBin = config.subprocessBin || path.resolve(__dirname, './bin/nyc.js')
this._tempDirectory = config.tempDirectory || './.nyc_output'

Choose a reason for hiding this comment

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

Why is path.resolve used on subprocessBin but not subprocessBin?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jameswomack I think the reasoning was that:

  • if you explicitly specify the path to the bin that should be run, we assume you know the path that you want.
  • otherwise we default to the bin that we know will be relative to index.js.

bcoe added a commit that referenced this pull request Feb 20, 2016
all of nyc's configuration options can now also be set in package.json
@bcoe bcoe merged commit 8b06898 into update-changelog Feb 20, 2016
@bcoe bcoe deleted the more-configurable branch February 20, 2016 20:16
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.

4 participants