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

Readme Reshuffle #1093

Merged
merged 12 commits into from May 3, 2019
Merged

Readme Reshuffle #1093

merged 12 commits into from May 3, 2019

Conversation

JaKXz
Copy link
Member

@JaKXz JaKXz commented Apr 28, 2019

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

… 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
@JaKXz JaKXz requested review from bcoe and coreyfarrell April 28, 2019 04:00
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 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 Show resolved Hide resolved
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) |
Copy link
Member

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.

Copy link
Member Author

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

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.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@JaKXz
Copy link
Member Author

JaKXz commented May 1, 2019

@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

JaKXz added 2 commits May 1, 2019 21:02
remove old custom require hooks section that has been updated above
README.md Outdated Show resolved Hide resolved
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.

LGTM!

@coreyfarrell coreyfarrell merged commit 65478be into master May 3, 2019
@coreyfarrell
Copy link
Member

Thanks for this great work @JaKXz!

@JaKXz JaKXz deleted the docs/configuration-reorg branch May 3, 2019 12:27
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

3 participants