tools/doc: improvements for types #11167

silverwind · 2017-02-04T17:08:17Z

Checklist

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

Affected core subsystem(s)

The first commit capitalizes all primitive types so the parser can pick them up.
The second commit adds logic to the doctool to allow parsing of Type[] array syntax.

Before

After

TimothyGu · 2017-02-04T18:47:23Z

The first commit LGTM. For the second, I would prefer a more generic syntax like Array<String>. This way, we can not only have links to MDN for both Array and String, but also have better symmetry with other language types/protocols like Promise and Iterable (e.g. for URLSearchParams it'd be Iterable<String, String>).

mscdex · 2017-02-04T22:39:39Z

I actually prefer primitives to stay lowercase. Can't we just fix the parser?

Also on an unrelated note, in the particular example you showed, the inline 'pem' property description should actually have Buffer instead of buffer too.

silverwind · 2017-02-04T23:50:06Z

I actually prefer primitives to stay lowercase

I don't have stats, but I estimate that > 80% of our types are currently capitalized.

the inline 'pem' property description should actually have Buffer instead of buffer too.

That'd be quite hard to parse. It's not following the type convention of {Type} at all.

mscdex · 2017-02-05T01:02:14Z

That'd be quite hard to parse. It's not following the type convention of {Type} at all.

I'm not suggesting it should parse it necessarily, I'm mostly just pointing out a typo that should be corrected.

lpinca · 2017-02-05T07:32:39Z

tools/doc/type-parser.js

@@ -62,6 +62,17 @@ module.exports = {
          typeUrl = nodeDocUrl + typeMap[typeText];
        }

+        if (/\[]$/.test(typeText)) {
+          const typeTextBare = typeText.slice(0, -2);
+          if (jsPrimitives[typeTextBare]) {


Can we merge this if...else chain with the previous one on line 57?

silverwind · 2017-02-05T11:23:24Z

So on current master we have 571 occurences in docs with capitalized primitive types and 160 without. I could change those all to lowercase as suggested by @mscdex. MDN currently lists them capitalized and the href would need to be capitalized as well.

thefourtheye · 2017-02-05T15:57:03Z

... the href would need to be capitalized as well.

I would consider this as a problem.

silverwind · 2017-02-14T20:42:45Z

Updated:

Primitive types are now all lowercased. This seems to be a common convention in JS documention.
Removed some duplicated code as per @lpinca's suggestion and made sure primitives of any case are picked up correctly by the parser.

Example:

@nodejs/documentation PTAL.

sam-github · 2017-02-14T22:00:37Z

Consistency sounds great to me.

Is the example in #11167 (comment) what it looks like now?

How come string is lower case, and Buffer and Object are upper case? It seems a bit random. Its not even built-in vs node specific, because String and Object are both builtin.

silverwind · 2017-02-14T22:17:22Z

Because stringis a primitive while the others are not.

TimothyGu · 2017-02-14T23:02:34Z

@silverwind, have you seen my comment above?
#11167 (comment)

silverwind · 2017-02-14T23:40:51Z

Yes, I think Array<String> make sense for non-Array types, and it's certainly something we can support in the parser. I'd just like to avoid packing too many changes in this PR. Maybe open an separate issue for it?

silverwind · 2017-02-15T11:18:31Z

While I couldn't find definitive recommendations, the style of lowercasing primitives (and capitalizing everything else) is in use by both JSDoc and closure compile. See examples here and here.

sam-github · 2017-02-16T16:34:39Z

Not so sure that's a useful distinction that we need to convey to our doc readers, given that strings do have methods in practical cases, but at least its something we can point to in our docs, and say we are following jsdoc.

Did you mean to write Array<String> , not Array<string>?

joyeecheung · 2017-02-27T04:18:39Z

BTW flow and TypeScript are using lower-cased primitive types too(therefore used by definitely typed, where a lot of people get their types from in their tools nowadays even if they don't use TypeScript)

TimothyGu · 2017-02-27T06:46:24Z

FWIW, Tern also uses lowercased primitive types.

thefourtheye · 2017-02-27T11:31:54Z

My mind is trained to understand a name as a constructor function if it starts with a capital letter. So I am going to go with lowercase names.

silverwind · 2017-02-27T17:15:51Z

I think we are in agreement that lowercase primitives is the way to go. Can I get a few LGTMs? I'll rebase and land shortly after it's approved.

joyeecheung

LTGM with a rebase

silverwind · 2017-03-02T16:57:33Z

Thanks! Rebased and landed in ff13619 and 9be03a2.

PR-URL: #11167 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins · 2017-05-15T15:32:12Z

ping

silverwind · 2017-05-16T15:30:12Z

Backport in #13054

PR-URL: #11167 Backport-PR-URL: #13054 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11167 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11167 Backport-PR-URL: nodejs#13054 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #11167 Backport-PR-URL: #13054 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

silverwind added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Feb 4, 2017

lpinca reviewed Feb 5, 2017

View reviewed changes

silverwind force-pushed the doc-types branch from 2189a90 to e8409ac Compare February 14, 2017 20:38

TimothyGu approved these changes Feb 27, 2017

View reviewed changes

jasnell approved these changes Feb 27, 2017

View reviewed changes

joyeecheung approved these changes Mar 2, 2017

View reviewed changes

silverwind added 2 commits March 2, 2017 17:53

doc: consistent case for primitive types

899a48b

doc: linkify type[] syntax, support lowercase for primitives

1ce91fd

silverwind force-pushed the doc-types branch from e8409ac to 1ce91fd Compare March 2, 2017 16:54

silverwind closed this Mar 2, 2017

silverwind added a commit that referenced this pull request Mar 2, 2017

doc: consistent case for primitive types

ff13619

PR-URL: #11167 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins added the backport-requested-v6.x label May 15, 2017

gibfahn added the backported-to-v6.x label May 16, 2017

gibfahn mentioned this pull request May 16, 2017

(v6.x backport) doc types fixes #13054

Closed

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools/doc: improvements for types #11167

tools/doc: improvements for types #11167

silverwind commented Feb 4, 2017

TimothyGu commented Feb 4, 2017

mscdex commented Feb 4, 2017 •

edited

Loading

silverwind commented Feb 4, 2017

mscdex commented Feb 5, 2017

lpinca Feb 5, 2017

silverwind commented Feb 5, 2017 •

edited

Loading

thefourtheye commented Feb 5, 2017

silverwind commented Feb 14, 2017 •

edited

Loading

sam-github commented Feb 14, 2017

silverwind commented Feb 14, 2017

TimothyGu commented Feb 14, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 15, 2017

sam-github commented Feb 16, 2017

joyeecheung commented Feb 27, 2017 •

edited

Loading

TimothyGu commented Feb 27, 2017

thefourtheye commented Feb 27, 2017

silverwind commented Feb 27, 2017

joyeecheung left a comment

silverwind commented Mar 2, 2017 •

edited

Loading

MylesBorins commented May 15, 2017

silverwind commented May 16, 2017

tools/doc: improvements for types #11167

tools/doc: improvements for types #11167

Conversation

silverwind commented Feb 4, 2017

Checklist

Affected core subsystem(s)

Before

After

TimothyGu commented Feb 4, 2017

mscdex commented Feb 4, 2017 • edited Loading

silverwind commented Feb 4, 2017

mscdex commented Feb 5, 2017

lpinca Feb 5, 2017

Choose a reason for hiding this comment

silverwind commented Feb 5, 2017 • edited Loading

thefourtheye commented Feb 5, 2017

silverwind commented Feb 14, 2017 • edited Loading

sam-github commented Feb 14, 2017

silverwind commented Feb 14, 2017

TimothyGu commented Feb 14, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 15, 2017

sam-github commented Feb 16, 2017

joyeecheung commented Feb 27, 2017 • edited Loading

TimothyGu commented Feb 27, 2017

thefourtheye commented Feb 27, 2017

silverwind commented Feb 27, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

silverwind commented Mar 2, 2017 • edited Loading

MylesBorins commented May 15, 2017

silverwind commented May 16, 2017

mscdex commented Feb 4, 2017 •

edited

Loading

silverwind commented Feb 5, 2017 •

edited

Loading

silverwind commented Feb 14, 2017 •

edited

Loading

joyeecheung commented Feb 27, 2017 •

edited

Loading

silverwind commented Mar 2, 2017 •

edited

Loading