tools: fix man pages linking regex#17724

DiegoRBaquero · 2017-12-18T02:47:21Z

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#17637Fixes: nodejs#17694 Refs: nodejs#17479

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

Trott

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

Trott · 2017-12-18T04:16:10Z

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.

lpinca · 2017-12-18T08:21:01Z

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

DiegoRBaquero · 2017-12-19T21:22:52Z

@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#17637Fixes: nodejs#17694 Refs: nodejs#17479

lpinca · 2017-12-19T22:17:11Z

tools/doc/html.js

+constdisplayAs=`${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}`+


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

DiegoRBaquero · 2017-12-20T16:32:00Z

@Trott @lpinca Updated

Trott

LGTM if CI is ok

lpinca · 2017-12-20T16:54:02Z

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

maclover7 · 2017-12-22T22:23:06Z

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.

maclover7 · 2017-12-22T22:24:45Z

Landed in 66e6aff

vsemozhetbyt · 2017-12-22T22:31:29Z

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

maclover7 · 2017-12-22T22:57:22Z

@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: #17724Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

vsemozhetbyt · 2018-01-22T15:15:55Z

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: #17724Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

gibfahn · 2018-01-24T01:39:07Z

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.

vsemozhetbyt · 2018-01-24T01:45:56Z

@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: #17724Fixes: #17694 Refs: #17479 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

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#17637Fixes: 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#17637Fixes: 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

Uh oh!

tools: fix man pages linking regex#17724

tools: fix man pages linking regex #17724

Uh oh!

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!

lpincaDec 19, 2017

Choose a reason for hiding this comment

Uh oh!

TrottDec 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!

lpincaDec 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

Trott commented Dec 18, 2017•
edited
Loading

TrottDec 19, 2017•
edited
Loading

maclover7 commented Dec 22, 2017•
edited
Loading