tools: fix man pages linking regex by DiegoRBaquero · Pull Request #17724 · nodejs/node · GitHub

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space.

Fixes: #17637
Fixes: #17694
Refs: #17479

commit message follows commit guidelines

Affected core subsystem(s)

tools, doc

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space. Fixes: nodejs#17637 Fixes: nodejs#17694 Refs: nodejs#17479

This add the space needed to match the man pages linking regex. Refs: nodejs#17724

I don't think this should be reverted. Rather the regular expression should be refined. Will comment more...

Maybe replace word boundary \b with an allowance for spaces or the beginning of a line (^|\s). That will stop it from replacing if the word break is a non-alpha symbol like [ and ( but still work at the start of a line.

@Trott's suggestion sounds good. In addition to that we should replace the space with the value of the first capturing group (^|\s)in the returned string.

@lpinca Indeed it would be needed, else a space would be removed.

The change to word boundary was breaking many doc pages. This replace the word boundary with a matching group of space or beginning of line. Fixes: nodejs#17637 Fixes: nodejs#17694 Refs: nodejs#17479

I think beginning should be used here, replacing the space.

@lpinca Same applies to line 427 too? Or am I mistaken about that?

yes, that's correct.

This moves the beginning regex matching group to the beginning of the resulting HTML man pages link

@Trott @lpinca Updated

LGTM if CI is ok

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

Landing for now as fixes main generation issue. Just noting for posterity that the curl and uname links referenced in #17637 are still broken, but it's for a different reason.

Landed in 66e6aff

@maclover7 Hmm. The links referenced in #17637 seem to not match the new RegExp, so they should not be erroneously double-linkified now if I do not miss something.

@vsemozhetbyt I think our comments crossed -- see #17637 (comment)

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space. PR-URL: #17724 Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

cc @nodejs/lts: Can this be backported to v8 branch?
See #17694 (comment)

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space. PR-URL: #17724 Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

cc @nodejs/lts: Can this be backported to v8 branch?

Done. I assume it doesn't need to go back to the v6.x branch.

@gibfahn Yes, it seems v6.x docs do not have this issue.

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space. PR-URL: #17724 Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tools: fix man pages linking regex

5af4822

The change to word boundary was breaking many doc pages. This reverts the word boundary back to space. Fixes: nodejs#17637 Fixes: nodejs#17694 Refs: nodejs#17479

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 18, 2017

DiegoRBaquero added a commit to DiegoRBaquero/node that referenced this pull request Dec 18, 2017

doc: fix man pages link to signal

147d180

This add the space needed to match the man pages linking regex. Refs: nodejs#17724

DiegoRBaquero mentioned this pull request Dec 18, 2017

doc: fix man pages link to signal #17725

Closed

2 tasks

gireeshpunathil approved these changes Dec 18, 2017

View reviewed changes

Trott requested changes Dec 18, 2017

View reviewed changes

DiegoRBaquero added 2 commits December 19, 2017 16:27

tools: fix man pages linking regex

fa42177

The change to word boundary was breaking many doc pages. This replace the word boundary with a matching group of space or beginning of line. Fixes: nodejs#17637 Fixes: nodejs#17694 Refs: nodejs#17479

tools: remove space in man pages returned link

b849956

lpinca reviewed Dec 19, 2017

View reviewed changes

tools: move beginning regex matching group

34a9db0

This moves the beginning regex matching group to the beginning of the resulting HTML man pages link

Trott approved these changes Dec 20, 2017

View reviewed changes

lpinca approved these changes Dec 20, 2017

View reviewed changes

jasnell approved these changes Dec 20, 2017

View reviewed changes

maclover7 self-assigned this Dec 22, 2017

maclover7 closed this Dec 22, 2017

maclover7 mentioned this pull request Dec 22, 2017

doc: autogenerated links with 404 status #17637

Closed

DiegoRBaquero deleted the patch-1 branch December 23, 2017 23:53

vsemozhetbyt mentioned this pull request Jan 4, 2018

Bug at API::Buffer::readable.read(0) nodejs/nodejs.org#1543

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

This was referenced Jan 22, 2018

doc: many erroneous linkifications #17694

Closed

v8.10.0 proposal #18336

Merged

MylesBorins added the dont-land-on-v6.x label Feb 27, 2018

+                    const displayAs = `${beginning}${name}(${number}${optionalCharacter})`;
                     if (BSD_ONLY_SYSCALLS.has(name)) {
-                      return ` <a href="https://www.freebsd.org/cgi/man.cgi?query=${name}` +
+                      return `<a href="https://www.freebsd.org/cgi/man.cgi?query=${name}` +

tools: fix man pages linking regex by DiegoRBaquero · Pull Request #17724 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: fix man pages linking regex - #17724

tools: fix man pages linking regex#17724DiegoRBaquero wants to merge 4 commits intonodejs:masternodejs/node:masterfrom DiegoRBaquero:patch-1DiegoRBaquero/node:patch-1Copy head branch name to clipboard

Conversation

DiegoRBaquero commented Dec 18, 2017

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Dec 18, 2017

Uh oh!

DiegoRBaquero commented Dec 19, 2017

Uh oh!

lpinca Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

DiegoRBaquero commented Dec 20, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Dec 20, 2017

Uh oh!

maclover7 commented Dec 22, 2017

Uh oh!

maclover7 commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Dec 22, 2017

Uh oh!

maclover7 commented Dec 22, 2017

Uh oh!

vsemozhetbyt commented Jan 22, 2018

Uh oh!

gibfahn commented Jan 24, 2018

Uh oh!

vsemozhetbyt commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tools: fix man pages linking regex#17724
DiegoRBaquero wants to merge 4 commits into
nodejs:masterfrom
DiegoRBaquero:patch-1

Trott commented Dec 18, 2017 •
edited
Loading

Trott Dec 19, 2017 •
edited
Loading

maclover7 commented Dec 22, 2017 •
edited
Loading