FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools/doc: improvements for types by silverwind · Pull Request #11167 · nodejs/node · GitHub

/ node Public

tools/doc: improvements for types - #11167

Closed
silverwind wants to merge 2 commits into
nodejs:masterfrom
silverwind:doc-types
Closed

tools/doc: improvements for types#11167
silverwind wants to merge 2 commits into
nodejs:masterfrom
silverwind:doc-types

Conversation

Copy link
Copy Markdown
Contributor
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

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

Copy link
Copy Markdown
Member

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 commented Feb 4, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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 commented Feb 5, 2017

Copy link
Copy Markdown
Contributor

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.

Comment thread tools/doc/type-parser.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind commented Feb 5, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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

I would consider this as a problem.

silverwind commented Feb 14, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

Because stringis a primitive while the others are not.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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 commented Feb 27, 2017
edited
Loading

Copy link
Copy Markdown
Member

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)

Copy link
Copy Markdown
Member

FWIW, Tern also uses lowercased primitive types.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LTGM with a rebase

silverwind commented Mar 2, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks! Rebased and landed in ff13619 and 9be03a2.

silverwind closed this Mar 2, 2017
silverwind added a commit that referenced this pull request Mar 2, 2017
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>

Copy link
Copy Markdown
Contributor

ping

Copy link
Copy Markdown
Contributor Author

Backport in #13054

gibfahn pushed a commit that referenced this pull request May 16, 2017
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>
gibfahn pushed a commit that referenced this pull request May 16, 2017
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>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
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>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
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 a commit to silverwind/node that referenced this pull request May 18, 2017
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>
silverwind added a commit to silverwind/node that referenced this pull request May 18, 2017
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>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
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>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
MylesBorins mentioned this pull request Jul 18, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.


Back | FazBrowse Home | New Git URL