★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/29496Nouvelle recherche | Portail wanayoo
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

doc: add code example to inspector.url() method #29496

Closed
wants to merge 5 commits into from
Closed

doc: add code example to inspector.url() method #29496

wants to merge 5 commits into from
+15 −0

Conversation

@juanarbol
Copy link
Member

juanarbol commented Sep 8, 2019

Checklist
@juanarbol
doc: add code example to inspector.url() method
Verified
This commit was signed with a verified signature.
juanarbol Juan José
GPG key ID: DE7887E676C9F8E9 Learn about signing commits
Loading status checks…
e1604a0
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Sep 8, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@juanarbol
fixup
Loading status checks…
843d69f
@cjihrig
cjihrig approved these changes Sep 9, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@juanarbol
fixup
Loading status checks…
9c75d5a
@lpinca
lpinca approved these changes Sep 12, 2019
View changes
@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Sep 24, 2019

ping @Trott
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Sep 25, 2019

LIte CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3928/
@Trott Trott added author ready and removed author ready labels Sep 25, 2019
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Sep 25, 2019

@nodejs/documentation How do we feel about this sample code? It's not really a runnable code sample. It's really just "Here's what the output would be with three completely different invocations of Node.js." I'm OK with this because it's probably still an improvement over the current doc. But there's probably an even better way to show this that doesn't suggest that the value can change within a single process when it won't?
@Trott Trott added the author ready label Sep 25, 2019
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Sep 25, 2019 •
edited

@Trott I'm not part of documentation working group, but I could change that JS sintax to Shell, so instead of:

// Node.js process was called with --inspect flag.
require('inspector').url()
Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
For help see https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

Could be more like:

$ node --inspect -p "require('inspector').url()"
Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
For help see https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Sep 26, 2019

@Trott I'm not part of documentation working group, but I could change that JS sintax to Shell, so instead of:

// Node.js process was called with --inspect flag.
require('inspector').url()
Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
For help see https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

Could be more like:

$ node --inspect -p "require('inspector').url()"
Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
For help see https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

I could see that being a good option, especially if the commands in question work verbatim on Windows.
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Sep 26, 2019

I'm not a Windows expert, but, I'd love to see the @nodejs/documentation opinions before making a new change, but yeah! I can change this for making this better.
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Sep 27, 2019

I'm not a Windows expert, but, I'd love to see the @nodejs/documentation opinions before making a new change, but yeah! I can change this for making this better.

I'm not a Windows expert, but I think those commands would work on Windows too. @nodejs/platform-windows
@Trott

This comment was marked as outdated.

Sign in to view
Copy link
Member

Trott commented Sep 27, 2019

Going to land this as-is (if CI passes) because it's an improvement to our current docs, but would love to see the suggested further improvements implemented and reviewed in another PR! Thanks for this one!
@Trott
Trott reviewed Sep 27, 2019
View changes
doc/api/inspector.md Outdated Show resolved Hide resolved
@Trott Trott removed the author ready label Sep 27, 2019
@juanarbol
fixup
Loading status checks…
b31502b
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Sep 28, 2019

I found something: "Return the URL of the active inspector, or undefined if there is none.", and by the way, In default options, I see that, by default, debugger is 127.0.0.1:9229, maybe, maybe... say that could improves documentation:

"Return the URL of the active inspector, 127.0.0.1:9229 by default, or undefined if there is none." With better sintax, of course.
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Oct 3, 2019

Pong @Trott
@Trott
Trott reviewed Oct 3, 2019
View changes
doc/api/inspector.md Outdated
@@ -52,6 +52,21 @@ parameter usage.

Return the URL of the active inspector, or `undefined` if there is none.

```console
$ node --inspect -p "require('inspector').url()"

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Oct 3, 2019

Member

Since built-in modules are automatically loaded when Node.js is run this way, we could consider simplifying to something like this here and below?:

Suggested change
$ node --inspect -p "require('inspector').url()"
$ node --inspect -p 'inspector.url()'

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Oct 3, 2019

Member

And adding to the suggested change above, we can make the output easier to read by making it clear what part of the output is from inspector.url() since there are a couple other lines of output that might confuse the reader?:

Suggested change
$ node --inspect -p "require('inspector').url()"
$ node --inspect -p '`\ninspector.url() result: ${inspector.url()}\n`'```

That makes the command line a little bit harder to read, but the output easier to read, so...
¯\(ツ)/¯
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Oct 3, 2019 •
edited

My comments above are suggestions only. If someone wants to run this on CI and land it as it is right now, I'm fine with that.
@juanarbol
fixup
Loading status checks…
f1aadae
@juanarbol

This comment has been minimized.

Sign in to view
Copy link
Member Author

juanarbol commented Oct 29, 2019

Ping @Trott, I think this is ready, is it?
@gireeshpunathil
gireeshpunathil approved these changes Oct 30, 2019
View changes
@BridgeAR
BridgeAR approved these changes Dec 9, 2019
View changes
@BridgeAR BridgeAR requested a review from Trott Dec 9, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Dec 9, 2019 •
edited by BridgeAR

CI: https://ci.nodejs.org/job/node-test-pull-request/27528/
Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4105/
@targos
targos approved these changes Dec 11, 2019
View changes
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Dec 11, 2019

Landed in 4113601
@targos targos closed this Dec 11, 2019
targos added a commit that referenced this pull request Dec 11, 2019
@juanarbol @targos
doc: add code example to inspector.url() method
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
4113601 
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Dec 13, 2019
@juanarbol @MylesBorins
doc: add code example to inspector.url() method
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
1cce000 
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
Merged
targos added a commit that referenced this pull request Jan 14, 2020
@juanarbol @targos
doc: add code example to inspector.url() method
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
b1358d3 
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs added a commit that referenced this pull request Feb 6, 2020
@juanarbol @BethGriggs
doc: add code example to inspector.url() method
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
cfe30ae 
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
@juanarbol Michael Bashurov
doc: add code example to inspector.url() method
94b6bb3 
PR-URL: nodejs#29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig

@lpinca lpinca

@targos targos

@gireeshpunathil gireeshpunathil

@BridgeAR BridgeAR

@Trott Trott

Assignees
No one assigned
Labels
author ready doc inspector
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@juanarbol @Trott @nodejs-github-bot @targos @lpinca @cjihrig @gireeshpunathil @BridgeAR
You can’t perform that action at this time.