| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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>). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
I don't have stats, but I estimate that > 80% of our types are currently capitalized.
That'd be quite hard to parse. It's not following the type convention of {Type} at all. |
Sorry, something went wrong.
I'm not suggesting it should parse it necessarily, I'm mostly just pointing out a typo that should be corrected. |
Sorry, something went wrong.
There was a problem hiding this comment.
Can we merge this if...else chain with the previous one on line 57?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
I would consider this as a problem. |
Sorry, something went wrong.
|
Updated:
Example: @nodejs/documentation PTAL. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Because stringis a primitive while the others are not. |
Sorry, something went wrong.
|
@silverwind, have you seen my comment above? |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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>? |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
FWIW, Tern also uses lowercased primitive types. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LTGM with a rebase
Sorry, something went wrong.
Sorry, something went wrong.
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>
|
ping |
Sorry, something went wrong.
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 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: 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>
| Back | FazBrowse Home | New Git URL |
Checklist
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