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

docs: instrument docs update #1063

Merged
merged 5 commits into from
Apr 22, 2019

Conversation

AndrewFinlay
Copy link
Contributor

Add documentation for --complete-copy, and describe how to instrument source JIT on an express server.

This should go some way to addressing #839, which looks like it's already implemented but not documented.

Add documentation for `--complete-copy`, and describe how to instrument source JIT on an express server.
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.

Crazy idea: in the interest of shortening and streamlining the readme to quick-start and configuration, can we actually move all the nyc instrument stuff to docs/instrument.md? That would be a really nice step towards focusing the readme.

@AndrewFinlay
Copy link
Contributor Author

@JaKXz that can be done, where do you want the link to go?

@JaKXz
Copy link
Member

JaKXz commented Apr 10, 2019

In the readme for now as a heading link, like the coveralls and codecov links

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 @AndrewFinlay! I have some suggestions... hopefully got everything...

docs/instrument.md Outdated Show resolved Hide resolved
docs/instrument.md Outdated Show resolved Hide resolved
docs/instrument.md Outdated Show resolved Hide resolved
docs/instrument.md Show resolved Hide resolved
docs/instrument.md Outdated Show resolved Hide resolved

The `--delete` option will remove existing output.

The `--complete-copy` option will copy all files from the `input` directory to the `output` directory.
Copy link
Member

Choose a reason for hiding this comment

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

Will it override the instrumented output? I think more specificity would be good here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems obvious to me, I don't see why you'd run nyc instrument to simply copy files and not instrument them. But it can change if it needs to.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I mean, I don't really understand from the wording... I assume the output will be input.js and input-instrumented.js but I don't know if that's actually right

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 for the updates, just one last change to make


## Streaming instrumentation

This form of the command will stream instrumented source directly to `stdout`, that can then be piped to another process.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence should just start with nyc instrument will stream...

Copy link
Member

Choose a reason for hiding this comment

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

@AndrewFinlay do you have a moment to address this so we can merge? If not I can make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been working on @JaKXz 's queries about how instrument handles absolute and relative paths in Windows, but life, family and Easter come first. I think this is the only remaining piece of work for this PR, and my gut feeling says that if Windows can't accept both relative and absolute paths that may be a bug anyway. So I'll get my Windows test env up and if I find anything that needs to change I'll pop it in a new PR,

Oh and thanks to all of you @JaKXz, @coreyfarrell & @bcoe for your help in getting these instrument changes over the line. It's been fun.

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'm a huge fan of starting to split docs into multiple files.

@bcoe
Copy link
Member

bcoe commented Apr 22, 2019

@JaKXz @AndrewFinlay, just bumping this up; what do we need to do to get this over the finish line?

@JaKXz JaKXz merged commit 56591fa into istanbuljs:master Apr 22, 2019
@JaKXz
Copy link
Member

JaKXz commented Apr 22, 2019

@bcoe done!

@AndrewFinlay AndrewFinlay deleted the docs-instrument-update branch May 10, 2019 02:41
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

4 participants