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

Add example usage to istanbul-lib-report #425

Merged
merged 3 commits into from Jul 3, 2019

Conversation

lukeapage
Copy link
Contributor

No description provided.

@coreyfarrell coreyfarrell requested a review from JaKXz June 21, 2019 11:17
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.

Thanks for starting this! Documentation in the monorepo is very much lacking, most documentation work has been on nyc itself for end-users. I have a couple comments and have requested a review by @JaKXz.

packages/istanbul-lib-report/README.md Outdated Show resolved Hide resolved
packages/istanbul-lib-report/README.md Outdated Show resolved Hide resolved
packages/istanbul-lib-report/README.md Outdated Show resolved Hide resolved
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

At first glance my major request is to have more descriptive variable names and even inline comments to explain the context and intent of the code and what it's doing.

Maybe another source for good examples would be jest's codebase, for inspiration :)

@coreyfarrell
Copy link
Member

Maybe another source for good examples would be jest's codebase, for inspiration

jest is currently not a good place to look. jest is using the current version of istanbul modules. index.js on istanbuljs/nyc#1134 is probably the only code that is using the next version of istanbul-lib-report.

@lukeapage
Copy link
Contributor Author

Fixed comments. I'm not saying this is perfect - its just I tried the alpha and updated my codebase and I ended up using the example on the refactor pr description, so I did this as I went along and made a PR so that something exists ;)

JaKXz
JaKXz previously requested changes Jun 24, 2019
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

Thanks @lukeapage - I have some minor things that I think would make things clearer - writing this review as a quick set of TODOs

packages/istanbul-lib-report/README.md Outdated Show resolved Hide resolved
packages/istanbul-lib-report/README.md Show resolved Hide resolved
packages/istanbul-lib-report/README.md Outdated Show resolved Hide resolved
packages/istanbul-lib-report/README.md Show resolved Hide resolved
@JaKXz JaKXz dismissed their stale review July 2, 2019 18:11

I'll leave this to @coreyfarell to merge.

@coreyfarrell coreyfarrell merged commit 3fe1e48 into istanbuljs:master Jul 3, 2019
@coreyfarrell
Copy link
Member

@lukeapage thanks for contributing!

@Hypnosphi
Copy link

Doesn't seem to work with istanbul-lib-report@2.0.8 and istanbul-reports@2.2.6:

TypeError: reports.create(...).execute is not a function

@coreyfarrell
Copy link
Member

@Hypnosphi that is true, these docs are for the 3.x versions (currently pre-release).

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

5 participants