Skip to content

doc: add comma and colon in message with --inspector doc url#19871

Closed
n-filatov wants to merge 2 commits into
nodejs:masterfrom
n-filatov:master
Closed

doc: add comma and colon in message with --inspector doc url#19871
n-filatov wants to merge 2 commits into
nodejs:masterfrom
n-filatov:master

Conversation

@n-filatov

Copy link
Copy Markdown
Contributor
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

When i follow link https://nodejs.org/en/docs/inspector i get 404 error. Replaced on https://nodejs.org/en/docs/guides/debugging-getting-started/ with inspect flag documentation

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Apr 7, 2018

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you drop the trailing slash from the URL? Otherwise, LGTM.

@n-filatov

Copy link
Copy Markdown
Contributor Author

@cjihrig Done

@richardlau

Copy link
Copy Markdown
Member

cc @nodejs/diagnostics

@richardlau

Copy link
Copy Markdown
Member

@cjihrig

cjihrig commented Apr 7, 2018

Copy link
Copy Markdown
Contributor

Good catch @richardlau. We should keep the existingURL and add the trailing slash, although I think a better solution is for the website to not require the trailing slash.

@n-filatov

Copy link
Copy Markdown
Contributor Author

Returned old URL but with trailing slash. Also created issue in nodejs.org repo to fix this bug.

@Trott

Trott commented Apr 8, 2018

Copy link
Copy Markdown
Member

Super minor nits, but while we're in here, can we add a comma after help and a period at the end in all of these? For help, see <URL>. If the period is undesirable, use a colon after see instead: For help, see: <URL>

@Trott

Trott commented Apr 8, 2018

Copy link
Copy Markdown
Member

(Also: Hi, @Muffassa! Welcome and thanks for the PR! These kinds of changes often attract lots of comments. Don't get discouraged! And thanks again!)

@n-filatov

Copy link
Copy Markdown
Contributor Author

@Trott No problem :) Fixed

@Trott

Trott commented Apr 8, 2018

Copy link
Copy Markdown
Member

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 8, 2018
@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 72 hours to land. label Apr 8, 2018
@richardlau

Copy link
Copy Markdown
Member

nodejs/build#1219 has fixed the redirect for the original URL.

@Trott

Trott commented Apr 9, 2018

Copy link
Copy Markdown
Member

nodejs/build#1219 has fixed the redirect for the original URL.

Would that make this PR unnecessary and close-able? (It seems that way to me, with apologies to @Muffassa for giving them nits/comments and then ultimately not accepting the change, but it happens.)

@Trott

Trott commented Apr 9, 2018

Copy link
Copy Markdown
Member

Oh, actually, those commas are still an improvement, so this could land, probably preferably with the slashes omitted though?

@Trott

Trott commented Apr 9, 2018

Copy link
Copy Markdown
Member

(Commit message will probably need to be updated too.)

@n-filatov n-filatov changed the title src: replace inspect flag documentation link doc: add comma and colon in message with --inspector doc url Apr 9, 2018
@n-filatov

Copy link
Copy Markdown
Contributor Author

Changed commit messages, removed slash in the end of url, changed request message.

@BridgeAR

BridgeAR commented Apr 9, 2018

Copy link
Copy Markdown
Member

@BridgeAR

BridgeAR commented Apr 9, 2018

Copy link
Copy Markdown
Member

Landed in f3e107a.

@Muffassa congratulations on your first commit to Node.js and to officially becoming a contributor! 🎉

@BridgeAR BridgeAR closed this Apr 9, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
PR-URL: nodejs#19871
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
cjihrig added a commit to cjihrig/node that referenced this pull request Apr 10, 2018
This commit adds basic functionality testing of the
help URL printed when the inspector starts.

PR-URL: nodejs#19887
Refs: nodejs#19871
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2018
PR-URL: #19871
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Apr 12, 2018
This commit adds basic functionality testing of the
help URL printed when the inspector starts.

PR-URL: #19887
Refs: #19871
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 72 hours to land. inspector Issues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants