-
-
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
Readme Reshuffle #1093
Readme Reshuffle #1093
Conversation
… ci] remove outdated docs and point more to the preset configs
add a Coverage thresholds section add the skip-full option to the table because it's interesting move nyc instrument link closer to the advanced features
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 is good work @JaKXz. Some things I'm pointing out pre-existing issues because the movement brought them to my attention. In those cases I'd say no harm deferring the fix, I just wanted to say something while I was thinking of it.
README.md
Outdated
| `check-coverage` | check whether coverage is within thresholds, fail if not | `Boolean` | `false` | | ||
| `extension` | a list of extensions that nyc should attempt to handle in addition to `.js` | `Array<String>` | `['.js']` | | ||
| `include` | [globs of files to be included from instrumentation](#selecting-files-for-coverage) | `Array<String>` | `['**']`| | ||
| `exclude` | See [selecting files for coverage for more info](#selecting-files-for-coverage) | `Array<String>` | [list](https://github.com/istanbuljs/istanbuljs/blob/master/packages/test-exclude/index.js#L176-L184) | |
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.
I'd say move for more info
out of the link.
For the default link the list is OK for now but I think we will need to move the defaults list in test-exclude to a separate file. The problem here is that any change to test-exclude code is likely to make this link point to the wrong line range.
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.
I think that will just have to change when that change happens in test-exclude
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.
I agree with @coreyfarrell this is great work 👍
Honestly, I'd rather than we don't use the npx
approach; let's keep the plain old nyc mocha
, but add a note about using npx
. My main reasons being:
- I often ask people to install from a
next
tag, which this doesn't work well with. - I don't like that it obfuscates the version of
nyc
that is being installed -- I like upgrading to be a more deliberate process.
Don't have any objection to you having this immediately below as an alternative approach.
@bcoe @coreyfarrell thanks for your feedback - I've made some changes and additions. Please try the new link and let me know what you think: https://github.com/istanbuljs/nyc/tree/82a6dd5 |
remove old custom require hooks section that has been updated above
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.
LGTM!
Thanks for this great work @JaKXz! |
The intent here is to make the readme much better for newcomers and getting started faster. The major change is promoting the config packages and moving the more advanced features and options towards the bottom. Let me know what you think!
Fixes ...many issues. Hopefully.
Resolves ...many questions. Hopefully.
Easier diff to read: https://github.com/istanbuljs/nyc/tree/82a6dd5