tools: improve heading type detection in json.js #20074

vsemozhetbyt · 2018-04-16T14:00:22Z

Checklist

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

It appears that heading type detection in tools/doc/json.js is not accurate because some RegExps are too narrow and some are too wide. Maybe some mean an older source state.

This is an incomplete list of examples of false-positive and false-negative results:

Not detected as constructor signatures (with Constructor: prefix or class parent object):

new assert.AssertionError(options)
new crypto.Certificate()
Constructor: new inspector.Session()
new net.Server([options][, connectionListener])
new net.Socket([options])
Constructor: new stream.Writable([options])
new stream.Readable([options])
new stream.Duplex(options)
new stream.Transform([options])
new tls.TLSSocket(socket[, options])
Constructor: new URL(input[, base])
Constructor: new URLSearchParams()
Constructor: new URLSearchParams(string)
Constructor: new URLSearchParams(obj)
Constructor: new URLSearchParams(iterable)
Constructor: new vm.Module(code[, options])
new vm.Script(code, options)

Not detected as methods signatures (with more than one ancestor):

fs.realpath.native(path[, options], callback)
fs.realpathSync.native(path[, options])
require.resolve.paths(request)
punycode.ucs2.decode(string)
punycode.ucs2.encode(codePoints)
util.types.isAnyArrayBuffer(value)
util.types.isArgumentsObject(value)
util.types.isArrayBuffer(value)

Wrongly detected as method signatures:

DEP0022: os.tmpDir()
DEP0023: os.getNetworkInterfaces()
DEP0026: util.print()
DEP0079: Custom inspection function on Objects via .inspect()

Not detected as properties (with more than one ancestor):

buffer.constants.MAX_LENGTH
buffer.constants.MAX_STRING_LENGTH
util.inspect.custom
util.inspect.defaultOptions
util.promisify.custom

Wrongly detected as properties:

No `require.extensions`
No `require.cache`
Node.js Crypto Constants
Node.js Error Codes
Native Abstractions for Node.js
V8 Inspector Integration for Node.js
DEP0011: crypto.Credentials
DEP0012: Domain.dispose

I've tried to make RegExp more accurate, but they became more and more complicated, so I've decided to build them from smaller string components so that the final RegExps were more self-explanatory.

vsemozhetbyt · 2018-04-19T09:50:48Z

cc @nodejs/documentation PTAL

I understand that this file is rather not very well researched domain, but the change does not require the familiarity with all the md-2-JSON tooling chain: it is just a refactoring of some RegExps that are used to parse well-known heading types in our API docs, so it can be reviewed without any broader context.

vsemozhetbyt · 2018-04-19T10:02:51Z

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

lpinca · 2018-04-22T06:16:50Z

tools/doc/json.js

+// To reduse escape slashes in RegExp string components.
+const r = String.raw;
+
+const eventPrefix = r`^Event:\s+`;


Any reason for \s? I mean, can there be tabs or new lines?

No, I do not think there can be. Do you mean it would be better to use '^Event: +'?

Done in the fixup commit.

lpinca · 2018-04-22T06:19:59Z

tools/doc/json.js

+
+// To include constructs like `readable\[Symbol.asyncIterator\]()`
+// or `readable.\_read(size)` (with Markdown escapes).
+const simpleId = r`(?:(?:\\?_)+|\b)\w+\b`;


Why is (?:\\?_) repeated? Is something like _\__\_ possible?

We have __filename and __dirname, so I've thought this was possible.

lpinca · 2018-04-22T06:38:16Z

tools/doc/json.js

+const classMethodPrefix = r`^Class Method:\s+`;
+const maybeClassPropertyPrefix = r`(?:Class Property:\s+)?`;
+
+const maybeQuote = '[\'"]?';


Use ` to avoid escaping?

I've tried, but ESLint throws. It seems this rule does not suffice for this:

node/.eslintrc.js

Line 217 in 2060787

'quotes': ['error', 'single', { avoidEscape: true }],

We need "allowTemplateLiterals": true and maybe I will file a PR with this very case if this is landed.

And I thought such PR would be a small one) #20214

vsemozhetbyt · 2018-04-22T11:23:19Z

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

lpinca

SGTM

vsemozhetbyt · 2018-04-22T13:03:45Z

@lpinca Thank you a lot for the review.

I will land soon if there are no objections from anybody as I think we may get no more reviews for this.

lpinca · 2018-04-22T13:08:22Z

I really appreciate all the work you are doing with the docs.

vsemozhetbyt · 2018-04-22T15:11:31Z

Landed in 7fcb52c
Thank you!

PR-URL: #20074 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 16, 2018

This comment has been minimized.

Sign in to view

vsemozhetbyt mentioned this pull request Apr 16, 2018

CI-lite does not send full info to PR #20072

Closed

tools: improve heading type detection in json.js

c536cde

lpinca reviewed Apr 22, 2018

View reviewed changes

fixup: replace \s with for strictness

c6c9709

lpinca approved these changes Apr 22, 2018

View reviewed changes

vsemozhetbyt closed this Apr 22, 2018

vsemozhetbyt added a commit that referenced this pull request Apr 22, 2018

tools: improve heading type detection in json.js

7fcb52c

PR-URL: #20074 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

vsemozhetbyt deleted the doc-tools-fix-json-types branch April 22, 2018 15:11

jasnell pushed a commit that referenced this pull request Apr 23, 2018

tools: improve heading type detection in json.js

1bdd3b0

PR-URL: #20074 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: improve heading type detection in json.js #20074

tools: improve heading type detection in json.js #20074

vsemozhetbyt commented Apr 16, 2018 •

edited

Loading

This comment has been minimized.

vsemozhetbyt commented Apr 19, 2018 •

edited

Loading

vsemozhetbyt commented Apr 19, 2018

lpinca Apr 22, 2018

vsemozhetbyt Apr 22, 2018

lpinca Apr 22, 2018

vsemozhetbyt Apr 22, 2018

lpinca Apr 22, 2018

vsemozhetbyt Apr 22, 2018 •

edited

Loading

lpinca Apr 22, 2018

vsemozhetbyt Apr 22, 2018 •

edited

Loading

vsemozhetbyt Apr 22, 2018

vsemozhetbyt commented Apr 22, 2018

lpinca left a comment

vsemozhetbyt commented Apr 22, 2018

lpinca commented Apr 22, 2018

vsemozhetbyt commented Apr 22, 2018

tools: improve heading type detection in json.js #20074

tools: improve heading type detection in json.js #20074

Conversation

vsemozhetbyt commented Apr 16, 2018 • edited Loading

Checklist

This comment has been minimized.

vsemozhetbyt commented Apr 19, 2018 • edited Loading

vsemozhetbyt commented Apr 19, 2018

lpinca Apr 22, 2018

Choose a reason for hiding this comment

vsemozhetbyt Apr 22, 2018

Choose a reason for hiding this comment

lpinca Apr 22, 2018

Choose a reason for hiding this comment

vsemozhetbyt Apr 22, 2018

Choose a reason for hiding this comment

lpinca Apr 22, 2018

Choose a reason for hiding this comment

vsemozhetbyt Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

lpinca Apr 22, 2018

Choose a reason for hiding this comment

vsemozhetbyt Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Apr 22, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 22, 2018

lpinca left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 22, 2018

lpinca commented Apr 22, 2018

vsemozhetbyt commented Apr 22, 2018

vsemozhetbyt commented Apr 16, 2018 •

edited

Loading

vsemozhetbyt commented Apr 19, 2018 •

edited

Loading

vsemozhetbyt Apr 22, 2018 •

edited

Loading

vsemozhetbyt Apr 22, 2018 •

edited

Loading