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

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

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

tools/doc/html.js

+      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}` +


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

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

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

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!

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

Trott commented Dec 18, 2017 •

edited

Loading

Trott Dec 19, 2017 •

edited

Loading

maclover7 commented Dec 22, 2017 •

edited

Loading