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: Add cache... options for instrument command #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndrewFinlay
Copy link

Makes the cache and cacheDir options available for the instrument command.

This change is a prerequisite to implementing this functionality in nyc. There will be a PR in the nyc repo accompanying this PR.

index.js Outdated
@@ -8,6 +8,7 @@ const nycCommands = {
instrument: [null, 'instrument'],
checkCoverage: [null, 'report', 'check-coverage'],
report: [null, 'report'],
cache: [null, 'instrument'],
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed as we can just use nycCommands.instrument (see suggested code changes below).

Copy link
Author

Choose a reason for hiding this comment

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

I've pushed up the recommended changes, but I'm still a little confused as to the purpose of the nycCommands block at the top of the schema file. What does it do?

Copy link
Member

Choose a reason for hiding this comment

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

It serves the same purpose as code such as const MAGIC_VALUE = 53;. Instead of repeating [null, 'instrument'] for each option we reference nycCommands.instrument. This module was created without much feedback so if you have suggestions for how to improve the code readability I'm 👍, but any such changes would need to be a separate PR.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Makes the 'cache' and 'cacheDir' options available for the instrument command
@coreyfarrell
Copy link
Member

@AndrewFinlay thanks for the PR. I'm buried with $dayjob work currently, it'll take me some time to move forward with merging this. Once the matching nyc PR is fixed I will try setting aside time to take another look.

@AndrewFinlay
Copy link
Author

No worries @coreyfarrell, that sounds good and I'm not in any particular rush. I'll try and progress it, take a look when you have some time.

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.

2 participants