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

Fix #7065: cli help documentation for --inspect #11660

Closed
wants to merge 1 commit into from
Closed

Fix #7065: cli help documentation for --inspect #11660

wants to merge 1 commit into from

Conversation

nojvek
Copy link

@nojvek nojvek commented Mar 2, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

documentation

@nodejs-github-bot nodejs-github-bot added cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. labels Mar 2, 2017
@nojvek
Copy link
Author

nojvek commented Mar 2, 2017

Context in: #7086

@nojvek
Copy link
Author

nojvek commented Mar 2, 2017

@joshgav @jasnell

@sam-github
Copy link
Contributor

needs changing in https://github.com/nodejs/node/blob/master/doc/node.1, too

@nojvek
Copy link
Author

nojvek commented Mar 2, 2017

is node.1 for man pages?

@sam-github
Copy link
Contributor

sam-github commented Mar 2, 2017 via email

@nojvek
Copy link
Author

nojvek commented Mar 3, 2017

Fixed.

doc/api/cli.md Outdated
Activate inspector on host:port. Default is 127.0.0.1:9229.

V8 Inspector integration allows attaching Chrome DevTools and IDEs to Node.js
instances for debugging and profiling. It uses the [Chrome Debugging Protocol][].
Copy link
Member

Choose a reason for hiding this comment

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

Do reference-style links work if they are linked in another file?

[Chrome Debugging Protocol]: https://chromedevtools.github.io/debugger-protocol-viewer/ is defined in debugger.md.

Copy link
Member

Choose a reason for hiding this comment

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

small, nit... this could be reworded as:

V8 Inspector integration uses the [Chrome Debugging Protocol]{} to attach
Node.js instances to debugging and profile tools such as Chrome DevTools
and various IDEs.

@nojvek
Copy link
Author

nojvek commented Mar 9, 2017 via email

@fhinkel
Copy link
Member

fhinkel commented Mar 9, 2017

make doc generates the docs, I think the html files end up in out/doc/api/. Do you want to check if we need to define
[Chrome Debugging Protocol]: https://chromedevtools.github.io/debugger-protocol-viewer/
in the file where you're using [Chrome Debugging Protocol]?

@nojvek
Copy link
Author

nojvek commented Mar 9, 2017 via email

@fhinkel
Copy link
Member

fhinkel commented Mar 9, 2017

Sorry if I wasn't clear. debugger.md uses and defines a link to the debugging protocol. If we're using a reference-style link, we need to define it in the file where we use it. We can just copy this definition to the end of cli.md.

Could you also fix-up your commit messages according to this guide please.

@nojvek
Copy link
Author

nojvek commented Mar 9, 2017 via email

doc/api/cli.md Outdated
@@ -94,6 +94,25 @@ Follows `require()`'s module resolution
rules. `module` may be either a path to a file, or a node module name.


### `--inspect[=host:port]`
<!-- YAML
added: v6.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think --inspect was initially added in 6.3.0. It might have changed slightly since then, but luckily we have per item changelogs now.

doc/api/cli.md Outdated

### `--inspect-brk[=host:port]`
<!-- YAML
added: v6.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was introduced in 7.6.0.

@nojvek
Copy link
Author

nojvek commented Mar 13, 2017 via email

@nojvek
Copy link
Author

nojvek commented Mar 14, 2017

@cjihrig Thanks for the version number verification. I wasn't quite 100% sure how to do that.

I have updated the PR with your changes. Looks great, the links work. Would love to see this merged.

image

@nojvek
Copy link
Author

nojvek commented Mar 14, 2017 via email

doc/api/cli.md Outdated
Activate inspector on host:port. Default is 127.0.0.1:9229.

V8 Inspector integration allows attaching Chrome DevTools and IDEs to Node.js
instances for debugging and profiling. It uses the [Chrome Debugging Protocol][].
Copy link
Member

Choose a reason for hiding this comment

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

small, nit... this could be reworded as:

V8 Inspector integration uses the [Chrome Debugging Protocol]{} to attach
Node.js instances to debugging and profile tools such as Chrome DevTools
and various IDEs.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 14, 2017

@nojvek any committer can land the PR. It should be good after @jasnell's nit is addressed. @fhinkel are you ok with it?

@nojvek
Copy link
Author

nojvek commented Mar 14, 2017 via email

doc/api/cli.md Outdated

Activate inspector on host:port. Default is 127.0.0.1:9229.

V8 inspector integration allows tools such as Chrome DevTools and IDEs to debug and profile Node.js instances. The tools attach to Node.js instances via a tcp port and communicate using the [Chrome Debugging Protocol][].
Copy link
Member

Choose a reason for hiding this comment

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

small nit here... need to line wrap a max 80 chars

@nojvek
Copy link
Author

nojvek commented Mar 15, 2017 via email

Adding documentation to node.1 and cli.md
@nojvek
Copy link
Author

nojvek commented Mar 16, 2017 via email

jasnell pushed a commit that referenced this pull request Mar 17, 2017
Adding documentation to node.1 and cli.md

PR-URL: #11660
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

Landed in 38299d2

@jasnell jasnell closed this Mar 17, 2017
@nojvek
Copy link
Author

nojvek commented Mar 17, 2017 via email

@nojvek nojvek deleted the cli-params-doc branch March 17, 2017 17:51
@joshgav
Copy link
Contributor

joshgav commented Mar 17, 2017

Thanks @nojvek!

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Adding documentation to node.1 and cli.md

PR-URL: nodejs#11660
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 21, 2017
Adding documentation to node.1 and cli.md

PR-URL: nodejs#11660
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants