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: fix header escaping regression (Issue #22065) #22084

Closed
wants to merge 5 commits into from

Conversation

rubys
Copy link
Member

@rubys rubys commented Aug 2, 2018

Remove backslashes in headers.

Fixes: #22065

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Aug 2, 2018
@@ -198,7 +198,8 @@ function preprocessElements({ filename }) {
heading.children = [{
type: 'text',
value: file.contents.slice(
position.start.offset, position.end.offset),
position.start.offset, position.end.offset)
.replace(/\\.{1}/g, (match) => match[1]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just:

.replace(/\\(?=.)/g, ''),

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, if we need to exclude escaped escapes:

.replace(/\\(?=[^\\])/g, ''),

Copy link
Member Author

Choose a reason for hiding this comment

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

jslint doesn't accept .replace(/\\(?=.)/g, ''),:

/Users/rubys/git/node/tools/doc/html.js
  202:29  error  Unescaped dot character in regular expression  node-core/no-unescaped-regexp-dot

I want to include escaped escapes.

Finally, lookahead isn't appropriate here. If we have an escaped escape, I want to emit a single backslash and then to not consider that backslash as an escape character. Consider four escapes in a row, the desired result would be two backslashes:

> "\\\\\\\\".replace(/\\(?=.)/g, '').length
1
> "\\\\\\\\".replace(/\\.{1}/g, (match) => match[1]).length
2

Copy link
Contributor

Choose a reason for hiding this comment

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

What about .replace(/\\(.{1})/g, '$1'),? Whould it be simpler and faster then function call and string index access?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 2, 2018

Choose a reason for hiding this comment

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

BTW, we can disable linter rule for a line to simplify RegExp, but I am not sure what is more confusing:

// eslint-disable-next-line node-core/no-unescaped-regexp-dot
.replace(/\\./g, (match) => match[1]),

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Not sure what is wrong with CI. Is it infrastructural fails?

@rubys
Copy link
Member Author

rubys commented Aug 2, 2018

+ sed '/^/\s*$/d'
sed: 1: "/^/\s*$/d": invalid command code \

Somebody either changed the version of sed or changed the build script. So, yes, I would say this looks like infrastructure fails.

Between the two, I would say that changing the build script seems considerably more likely. The second slash just looks outright wrong. Perhaps they meant to have a backslash escaping the next backslash so that they shell would pass sed a regular expression containing only a single backslash. If that is indeed the problem, it would seem amusing given the change that this particular pull request is addressing. :-)

@vsemozhetbyt

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Aug 2, 2018

@richardlau
Copy link
Member

Looks to be a real failure:

17:58:34 /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/doc/html.js:203
17:58:34             position
17:58:34             ^^^^^^^^
17:58:34 
17:58:34 SyntaxError: Unexpected identifier
17:58:34     at new Script (vm.js:72:7)
17:58:34     at createScript (vm.js:234:10)
17:58:34     at Object.runInThisContext (vm.js:286:10)
17:58:34     at Module._compile (internal/modules/cjs/loader.js:657:28)
17:58:34     at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
17:58:34     at Module.load (internal/modules/cjs/loader.js:599:32)
17:58:34     at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
17:58:34     at Function.Module._load (internal/modules/cjs/loader.js:530:3)
17:58:34     at Module.require (internal/modules/cjs/loader.js:637:17)
17:58:34     at require (internal/modules/cjs/helpers.js:20:18)
17:58:34 Building addon in /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/addons/05_wrapping_c_objects
17:58:34 make[2]: *** [out/doc/api/path.html] Error 1

https://ci.nodejs.org/job/node-test-commit-linuxone/3483/nodes=rhel72-s390x/consoleFull

@@ -198,7 +198,8 @@ function preprocessElements({ filename }) {
heading.children = [{
type: 'text',
value: file.contents.slice(
position.start.offset, position.end.offset),
position.start.offset, position.end.offset)
.replace(/\\(.{1})/g, '$1')
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing comma?

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt vsemozhetbyt added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 3, 2018
@vsemozhetbyt
Copy link
Contributor

It seems we have one more similar regression case: HTML entities verbatim rendering:

https://nodejs.org/download/nightly/v11.0.0-nightly20180803d68f946fe4/docs/api/inspector.html#inspector_event_lt_inspector_protocol_method_gt

Not sure if this should be fixed in this PR or in a new one.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2018

@rubys ... just a nit the prefix for the commit message here should be tools: rather than doc:

Also, just as a convention, it is helpful to include a [Squash] or [Fixup] prefix on commits that need to be squashed prior to landing.

@jasnell jasnell changed the title Issue 22065 doc: fix header escaping regression (Issue #22065) Aug 3, 2018
@rubys
Copy link
Member Author

rubys commented Aug 5, 2018

My pull request for https://www.npmjs.com/package/mdast-util-to-hast has been accepted. Updating to the latest version of this dependency should fix everything. In fact, I should be able to back out some of the work that is currently committed. I'll run some tests and see what needs to be done from there.

@rubys rubys mentioned this pull request Aug 7, 2018
4 tasks
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

@rubys ... is this ready to go?

@rubys
Copy link
Member Author

rubys commented Aug 12, 2018

@jasnell: this PR is an accumulated set of workarounds to a remark bug, it certainly can go in now, but will all need to be backed out when these pull requests land: #22140 (comment)

Preferred long term fix can be found at:
nodejs#22140
@rubys
Copy link
Member Author

rubys commented Aug 13, 2018

@rubys
Copy link
Member Author

rubys commented Aug 13, 2018

My read is that the freebsd failure is unrelated. Unless there are any objections, I'll land this PR tomorrow.

rubys added a commit that referenced this pull request Aug 13, 2018
quick fix for #22065

Preferred long term fix can be found at:
#22140

PR-URL: #22084
Fixes: #22065
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@rubys
Copy link
Member Author

rubys commented Aug 13, 2018

Landed in 59f8276

@rubys rubys closed this Aug 13, 2018
rvagg pushed a commit that referenced this pull request Aug 15, 2018
quick fix for #22065

Preferred long term fix can be found at:
#22140

PR-URL: #22084
Fixes: #22065
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: header escaping regression
7 participants