-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 more type case inconsistencies #11697
Conversation
doc/api/buffer.md
Outdated
@@ -508,7 +508,7 @@ added: v5.10.0 | |||
--> | |||
|
|||
* `size` {Integer} The desired length of the new `Buffer` | |||
* `fill` {string | Buffer | Integer} A value to pre-fill the new `Buffer` with. | |||
* `fill` {string|Buffer|Integer} A value to pre-fill the new `Buffer` with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer
needs to be fixed as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer
is not a valid type. Shall we change them all to number
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to be specific to keep Integer
here because it doesn’t really make sense to pass in non-integer number
s here… the upper-casing is a bit weird but I’d be okay with it, as long as we try to be consistent
(also /cc @ameliavoncat whose awesome PRs also touch these parts of the docs and who might want to keep track of this discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess it's better to keep it as-is. Per our rule discussed in the last PR, everything except the six primitives should be uppercased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind in your commit that changed the case in the doctool, Integer
was changed to integer
. Either way it doesn't seem right to me that number and integer have different cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll move it out of jsPrimitives
and into typeMap
, possibly linking it to number
docs.
doc/api/crypto.md
Outdated
* `key` : {String} - PEM encoded private key | ||
* `passphrase` : {String} - passphrase for the private key | ||
* `key` : {string} - PEM encoded private key | ||
* `passphrase` : {string} - passphrase for the private key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you drop the spaces before the colons in this file? We don’t do that anywhere else in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d94efe0
to
d5d4274
Compare
@TimothyGu d5d4274 fixes the |
But the issue at hand is that an integer is expressed as a number primitive. I also just looked at a proposal in TypeScript to add integral types, and though not yet accepted, it also uses lower cased type name ( |
@TimothyGu okay, lowercased |
6acf5f0
to
2d3068b
Compare
2d3068b
to
8ad63ec
Compare
Landed in 5f32024. |
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: #11697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: nodejs#11697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: nodejs#11697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: nodejs#11697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Do we want to backport to v6.x? |
@silverwind would you be willing to put together a PR with those commits? |
@MylesBorins not right now, I'm busy on other projects. But if you can wait a bit, I can put something together later. |
Backport in #13054 |
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: nodejs#11697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
- fix a number of uppercase types - lowercase 'integer' - consistent formatting in crypto PR-URL: nodejs#11697 Backport-PR-URL: nodejs#13054 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesTurns out I missed quite a few types in ff13619 because my
sed
skills weren't up to par. This commit should fix all remaining issues with primitive type cases.I also made the syntax with multiple types consistent by removing any whitespace between curly braces. If more people prefer
{type | type | type}
over{type|type|type}
, I can also change to that.Type replacement was done with: