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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

n-filatov
Copy link
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 approved these changes Apr 7, 2018
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

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
Contributor Author

@cjihrig Done

@richardlau
Copy link
Member

cc @nodejs/diagnostics

@richardlau
Copy link
Member

https://nodejs.org/en/docs/inspector/ redirects to https://nodejs.org/en/docs/guides/debugging-getting-started/ but https://nodejs.org/en/docs/inspector (without the trailing slash) 404's.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 7, 2018 •
edited
Loading

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
Contributor Author

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

@Trott
Copy link
Member

Trott commented Apr 8, 2018

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
Copy link
Member

Trott commented Apr 8, 2018

(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
Contributor Author

@Trott No problem :) Fixed

@Trott
Copy link
Member

Trott commented Apr 8, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14125/

@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 48 hours to land. label Apr 8, 2018
@cjihrig cjihrig mentioned this pull request Apr 9, 2018
Merged
3 tasks
@richardlau
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Apr 9, 2018

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
Copy link
Member

Trott commented Apr 9, 2018

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

@Trott
Copy link
Member

Trott commented Apr 9, 2018

(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
Contributor Author

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

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

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
@n-filatov @BridgeAR
src: add punctuation in --inspector doc url message
f3e107a 
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
@cjihrig
test: verify inspector help url works
dfea13a 
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
@n-filatov @targos
src: add punctuation in --inspector doc url message
95ab327 
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
@cjihrig @targos
test: verify inspector help url works
2ef3380 
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
Reviewers

@ofrobots ofrobots ofrobots approved these changes

@Trott Trott Trott approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@marswong marswong marswong approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
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 48 hours to land. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@n-filatov @richardlau @cjihrig @Trott @BridgeAR @ofrobots @TimothyGu @marswong @trivikr @nodejs-github-bot