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

Fixes #7065: cli help documentation for --debug and --debug-brk #7086

Closed
wants to merge 1 commit into from
Closed

Fixes #7065: cli help documentation for --debug and --debug-brk #7086

wants to merge 1 commit into from

Conversation

nojvek
Copy link

@nojvek nojvek commented Jun 1, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Cli documentation for --debug and --debug-brk when node -h is invoked.

cc @Fishrock123

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 1, 2016
added: v6.2.0
-->

listen for debugger commands on user specified port. If port is not specified e.g `--debug` then use 5858 as default.
Copy link
Member

Choose a reason for hiding this comment

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

Long line (80 columns max) and sentences should be properly capitalized. The version number is not correct but I don't really remember when they were added.

@bnoordhuis
Copy link
Member

I'd advise against documenting --debug-brk for now. It doesn't really work as advertised and its behavior may change, see #3589.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 1, 2016

node debug should really have it's own node-debug.1 man page, fwiw.

@nojvek
Copy link
Author

nojvek commented Jun 1, 2016

Debug-Brk works for 99% of use cases right? Should an edge case stop it
from being advertised?

Regarding node-debug.1 page, what should go into it?

On Wednesday, June 1, 2016, Jeremiah Senkpiel notifications@github.com
wrote:

node debug should really have it;s own node-debug.1 man page, fwiw.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7086 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA-JVI0BQn6QjS7GJ0aoTWuEjogPIsBeks5qHZXLgaJpZM4IrEx_
.

@bnoordhuis
Copy link
Member

Debug-Brk works for 99% of use cases right? Should an edge case stop it from being advertised?

Yes, because how it works is almost certainly going to change. If it's documented, we risk breaking code that depends on the current behavior.

@nojvek
Copy link
Author

nojvek commented Jun 1, 2016

makes sense. Thanks for the explanation.

So the plan is to Remove debug-brk, and keep --debug right ?

Once you confirm, I'll send an update to the PR.

On Wed, Jun 1, 2016 at 8:57 AM, Ben Noordhuis notifications@github.com
wrote:

Debug-Brk works for 99% of use cases right? Should an edge case stop it
from being advertised?

Yes, because how it works is almost certainly going to change. If it's
documented, we risk breaking code that depends on the current behavior.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7086 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA-JVAE-qvUmRbcnstix9UUtBqiDnKGfks5qHavvgaJpZM4IrEx_
.

@bnoordhuis
Copy link
Member

So the plan is to Remove debug-brk, and keep --debug right ?

Yes.

@owenallenaz
Copy link

Looking at the referenced issue #3589 it appears that was solved and closed. With that in mind is --debug-brk still a feature that is going to be cut? I find value in the feature and was hoping to fix the cluster module to properly support it since waiting until a debugger has attached is essential for handling debugging content on start-up and I'm not aware of another way of tackling it.

@joshgav
Copy link
Contributor

joshgav commented Jan 9, 2017

Oh, I didn't notice this PR and added the CLI docs in the PR for --inspect-brk: #8979.

@nojvek want to update the rest of the docs to target "inspect" instead and then land? Thank you!

@nojvek
Copy link
Author

nojvek commented Jan 9, 2017 via email

@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

@nodejs/v8-inspector ... is this still needed?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@joshgav
Copy link
Contributor

joshgav commented Mar 1, 2017

We don't need docs for --debug anymore, but do need to add --inspect and node inspect to the same file (./doc/api/cli.md). @nojvek would be great if you want to do that, I guess in a new PR. Thank you!

If we want to write a man page for node inspect as proposed above for node debug I think it belongs in the node-inspect repo.

@nojvek
Copy link
Author

nojvek commented Mar 1, 2017 via email

@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

I assume then that it's ok to close this PR?

@nojvek
Copy link
Author

nojvek commented Mar 2, 2017

@jasnell you can close this PR.

@joshgav made new PR for cli.md inspect documentation #11660 . Please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants