Skip to content

/node

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
wants to merge 4 commits into from
+2 −2

Conversation

@sleekweasel
Copy link

commented Jun 24, 2019

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.
Tim Baverstock
Add documentation line for host:port invocation
Loading status checks…
5d18c48
@gireeshpunathil
gireeshpunathil approved these changes Jun 24, 2019
View changes
@richardlau
richardlau reviewed Jun 24, 2019
View changes
lib/internal/main/print_help.js Outdated Show resolved Hide resolved
@Trott
Trott requested changes Jun 24, 2019
View changes
Copy link
Member

left a comment

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.
@sleekweasel @richardlau
Update lib/internal/main/print_help.js 
Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
128f181
@sleekweasel

This comment has been minimized.

Sign in to view
Copy link
Author

commented Jun 25, 2019

Argh, I can't believe I conflated localhost with the 192 network! It was a stressful, frustrating day.
@sleekweasel
Remove extraneous line
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4296fa5
@Trott
Trott reviewed Jun 25, 2019
View changes
lib/internal/main/print_help.js Outdated
@@ -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' +

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Jun 25, 2019

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Jul 6, 2019

Member

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....
@Trott
Trott approved these changes Jun 25, 2019
View changes
Copy link
Member

left a comment

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.)
@richardlau
richardlau approved these changes Jun 25, 2019
View changes
@BridgeAR
BridgeAR approved these changes Jul 4, 2019
View changes
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 4, 2019

Lite CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3737/

@BridgeAR BridgeAR added author ready lib / src labels Jul 4, 2019

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

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
fixup
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
dbc9f71
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3744/
@richardlau
richardlau approved these changes Jul 6, 2019
View changes
@trivikr
trivikr approved these changes Jul 10, 2019
View changes
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 12, 2019

Landed in 7008d78

@Trott Trott closed this Jul 12, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 12, 2019
Tim Baverstock @Trott
doc: add line for inspect host:port invocation 
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>
7008d78
targos added a commit that referenced this pull request Jul 20, 2019
Tim Baverstock @targos
doc: add line for inspect host:port invocation 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
74af944
This was referenced Jul 23, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott

@richardlau richardlau

@gireeshpunathil gireeshpunathil

@BridgeAR BridgeAR

@trivikr trivikr

Assignees
No one assigned
Labels
author ready lib / src
Projects
None yet
Milestone
No milestone
6 participants
@sleekweasel @BridgeAR @Trott @richardlau @gireeshpunathil @trivikr
You can’t perform that action at this time.