-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Make update to caching information in readme #968
Make update to caching information in readme #968
Conversation
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Going to wait for a second approval before merging as grammar is not my strongest area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one small question this looks great, thanks!
README.md
Outdated
You can run `nyc` with the optional `--cache` flag, to prevent it from | ||
instrumenting the same files multiple times. This can significantly | ||
improve runtime performance. | ||
`nyc`'s default behavior is to cache instrumented files to disk to prevent instrumenting source files multiple times, and speed `nyc` execution times. You can disable this behavior by running `nyc` with the `--cache false` flag. You can also change the default cache directory from `./node_modules/.cache/nyc` by setting the `--cacheDir` flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it --cache-dir
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out... that while this is processed with the rest of the config, it's not actually specified in the yargs
-- which I assume means won't come through. Assumedly it can be specified from .nycrc
files though.
https://github.com/istanbuljs/nyc/blob/master/lib/config-util.js#L105-L111
- I can add this to the
yargs
settings. - I can remove the reference to it and let someone else work on this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to see it added! Cc @bcoe
@JaKXz @coreyfarrell, etc -- I think I've made the necessary updates, please let me know if you need anything else from me! |
The scope of this PR has expanded since this review
I'll leave this to @bcoe or @coreyfarrell to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me 👍
Since #454, caching has been enabled by default but the README does not reflect this. Our team lost some time following the README, so contributing back our findings for the next team!