Skip to content

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

Merged
bcoe merged 2 commits into
update-changelogfrom
more-configurable
Feb 20, 2016
Merged

all of nyc's configuration options can now also be set in package.json#167
bcoe merged 2 commits into
update-changelogfrom
more-configurable

Conversation

@bcoe

@bcoe bcoe commented Feb 14, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
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

bcoe commented Feb 15, 2016

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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

bcoe commented Feb 15, 2016

Copy link
Copy Markdown
Member Author

@lloydcotten do you caching turned on?

@lloydcotten

Copy link
Copy Markdown
Contributor

Yes, I had caching on.

@novemberborn

Copy link
Copy Markdown
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
Copy Markdown
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

bcoe commented Feb 15, 2016

Copy link
Copy Markdown
Member Author

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

I'll make this nyc@6.0.0, since it changes config's functionality quite a bit.

@novemberborn

Copy link
Copy Markdown
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.

Comment thread index.js
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'

Copy link
Copy Markdown

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
Copy Markdown
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