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 documentation line for host:port invocation #28405

Closed

Conversation

sleekweasel
Copy link

I just wasted a load of time not knowing how to invoke 'node inspect' with host:port to debug a process started with --inspect-port=0 because it didn't show up from a lot of Googling. Having it in the help text would have saved that.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Welcome @sleekweasel and thanks for the pull request. This looks good to me once the hard-coded IP address is removed per one of @richardlau's suggestions.

Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
@sleekweasel
Copy link
Author

Argh, I can't believe I conflated localhost with the 192 network! It was a stressful, frustrating day.

@@ -164,7 +164,7 @@ function print(stream) {
options.set('--', { helpText: 'indicate the end of node options' });
stream.write(
'Usage: node [options] [ -e script | script.js | - ] [arguments]\n' +
' node inspect script.js [arguments]\n\n' +
' node inspect [script.js | -e "script" | <host>:<port>] …\n\n' +
Copy link
Member

Choose a reason for hiding this comment

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

This uses an ellipsis character () where everywhere else, we use three dots (...). On the one hand, the ellipsis is more correct. On the other hand, is there a situation where it will be an unprintable character because the terminal only accepts ASCII? Regardless, the code should probably pick one or the other and stick with it.

Copy link
Member

Choose a reason for hiding this comment

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

Linter requires non-ASCII characters (to keep binary size down, believe it or not) so ... it must be.

Also, this should probably align with line 166 in that no " around script and -e comes before the script.js option.

Assuming you left the default checked state for the checkbox about allowing collaborators in this repo to push to your branch, I'll make these updates right now....

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with or without the ellipsis comment being addressed. (It can be addressed in a later pull request if it doesn't get addressed in this one. Might even be a good first contribution for someone.)

@BridgeAR
Copy link
Member

BridgeAR commented Jul 4, 2019

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 4, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jul 4, 2019

The commit message should be updated either before landing or while landing and it should include the subsystem (Seems like lib would suffice here).

@Trott
Copy link
Member

Trott commented Jul 6, 2019

I got rid of -e and - because they are already mentioned in the options list. Folks who reviewed, please take a look. Whoever lands this (I volunteer!) will have to edit that info into the commit message.

@BridgeAR @gireeshpunathil @richardlau

@Trott
Copy link
Member

Trott commented Jul 6, 2019

@Trott
Copy link
Member

Trott commented Jul 12, 2019

Landed in 7008d78

@Trott Trott closed this Jul 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 12, 2019
PR-URL: nodejs#28405
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Jul 20, 2019
PR-URL: #28405
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants