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

feat: instrument --complete-copy implementation #1056

Merged
merged 6 commits into from Apr 9, 2019

Conversation

AndrewFinlay
Copy link
Contributor

So this was split out from the nyc instrument --include --exclude work.

When running nyc instrument <input> <output> this will copy everything across from input to output, excluding the .git and <output> folders.

There are a few caveats though.

  • This implementation will dereference symlinks as it copies them, and in some cases this could be a problem i.e. node_modules/.bin.
  • This also will not copy across empty directories, but this wouldn't be too hard to remedy.

If I can't find a good fix for the symlinks problem, my gut feeling on this one is that this should probably be an opt in with a --full switch.

Andrew Finlay added 2 commits April 5, 2019 09:55
With a few caveats:
This will dereference symlinks as it copies them, in some cases this could be a problem i.e. `node_modules/.bin`
This will not copy across empty directories
@coreyfarrell
Copy link
Member

If I can't find a good fix for the symlinks problem, my gut feeling on this one is that this should probably be an opt in with a --full switch.

I think --complete-copy would be good / impossible to confuse flag, this has actually been requested at #678.

…haviour

This default for `--complete-copy` has been set to false, meaning only instrumented files will be copied to the output directory in the default case.
@AndrewFinlay AndrewFinlay changed the title feat: instrument full copy implementation feat: instrument --complete-copy implementation Apr 5, 2019
Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

This looks pretty good, just the one little tweak.

index.js Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.179% when pulling a12d0d6 on AndrewFinlay:instrument-copy-full into e597c46 on istanbuljs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.179% when pulling a12d0d6 on AndrewFinlay:instrument-copy-full into e597c46 on istanbuljs:master.

bcoe
bcoe previously requested changes Apr 9, 2019
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.

this is looking like a great start 👍 thanks for the work @AndrewFinlay.

my major feedback is just that index.js continues to get more complex, and I think it's potentially worth tearing out some of the instrument logic into a lib/instrument helper.

I also wish we didn't need to pull in the cpr module; but it does seem like this would be a pain to implement ourselves -- underscores that cp --recursive would be nice to add to Node.js.

@@ -194,7 +196,15 @@ NYC.prototype.instrumentAllFiles = function (input, output, cb) {
const stats = fs.lstatSync(input)
if (stats.isDirectory()) {
inputDir = input
this.walkAllFiles(input, visitor)

const filesToInstrument = this.exclude.globSync(input)
Copy link
Member

Choose a reason for hiding this comment

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

instrumentAllFiles is getting to be a pretty large method. I'd be tempted to potentially move this logic into lib/instrument and split it into a couple helper methods.

@coreyfarrell coreyfarrell dismissed bcoe’s stale review April 9, 2019 23:05

we're going to defer refactoring for post 14.0.0

@coreyfarrell coreyfarrell merged commit 2eb13c6 into istanbuljs:master Apr 9, 2019
@coreyfarrell coreyfarrell mentioned this pull request Apr 9, 2019
@AndrewFinlay AndrewFinlay deleted the instrument-copy-full branch April 10, 2019 00:03
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.

None yet

4 participants