Skip to content

Conversation

@noel046
Copy link

@noel046noel046 commented Mar 14, 2023

Fixes: #47070

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-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
This was referenced Mar 21, 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

@noel046noel046 changed the title update incorrect links #47070doc: update incorrect linksMar 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

@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.

@TrottTrottforce-pushed the Incorrect-links-#47070 branch from f9629a7 to b358df1CompareMarch 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.

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

@TrottTrottApr 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

Please also add a

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

to the commit message.

@mscdexmscdex mentioned this pull request Apr 8, 2023
@mscdexmscdex mentioned this pull request Apr 22, 2023
@mscdex
Copy link
Contributor

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

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

@mscdex
Copy link
Contributor

@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

Superseded by #48194.

@aduh95aduh95 closed this Sep 20, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect links

7 participants

@noel046@nodejs-github-bot@deokjinkim@Trott@mscdex@ovflowd@aduh95