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

doc: update incorrect links #47085

Closed
wants to merge 1 commit into from

Conversation

noel046
Copy link

@noel046 noel046 commented Mar 14, 2023

Fixes: #47070

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 14, 2023
@deokjinkim
Copy link
Contributor

@noel046 Thank you for contribution. Commit title needs to be started with subsystem like doc: update incorrect links. Please follow Commit message guidelines.
https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines

@noel046 noel046 changed the title update incorrect links #47070 doc: update incorrect links Mar 24, 2023
@deokjinkim
Copy link
Contributor

@noel046 For now, PR title is only changed. But we need to change commit title(not PR of github) in git.

@Trott
Copy link
Member

Trott commented Mar 26, 2023

@noel046 For now, PR title is only changed. But we need to change commit title(not PR of github) in git.

The merge commit will need to be removed too. I'll do that now and force push.

@Trott Trott force-pushed the Incorrect-links-#47070 branch from f9629a7 to b358df1 Compare March 26, 2023 02:59
@Trott
Copy link
Member

Trott commented Apr 1, 2023

The CI failure is relevant. This breaks links in the single-page version of the doc.

[`'error'`]: #event-error_1
[`'error'`]: #event-error-1
Copy link
Member

@Trott Trott Apr 3, 2023

Choose a reason for hiding this comment

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

The current anchor exists in our HTML docs. The new one does not. What is the motivation for this change? Is it to fix something in the GitHub markdown rendering of this doc?

Copy link
Member

Choose a reason for hiding this comment

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

+1, on our tooling we replace dots with nothing and underscores with hyphens. You can see it here: https://github.com/nodejs/node/blob/main/tools/doc/html.mjs#L426 (We do the same on nodejs.dev here https://github.com/nodejs/nodejs.dev/blob/main/util-node/createSlug.js#L1)

This was referenced Apr 7, 2023
@mscdex
Copy link
Contributor

mscdex commented Apr 7, 2023

Please also add a

Fixes: https://github.com/nodejs/node/issues/47070

to the commit message.

@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2023

Can we get this merged soon? We've been getting more and more identical PRs for the same (or a subset of) changes.

@ovflowd
Copy link
Member

ovflowd commented Apr 22, 2023

@mscdex did the issues Trott and me mentioned get resolved?

@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2023

@ovflowd I don't know. However from my recollection we tend to give the first to submit a PR for a specific set of changes priority, but as this PR has been open for over a month with others willing to make the same changes, we need to decide whether to continue waiting or close this PR and give others a chance to make the changes needed.

@aduh95
Copy link
Contributor

aduh95 commented Sep 20, 2023

Superseded by #48194.

@aduh95 aduh95 closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect links
7 participants