doc,tool: refactor checking jsPrimitives #8740
Conversation
|
Manually added |
| if (jsPrimitives.indexOf(typeText) !== -1) { | ||
| typeUrl = jsPrimitiveUrl + '#' + typeText + '_type'; | ||
| const primitive = jsPrimitives[typeText]; | ||
| if (primitive !== undefined) { |
There was a problem hiding this comment.
Any reason for explicitly checking for !== undefined? Isn't if (primitive) sufficient?
There was a problem hiding this comment.
Checking with undefined might be more precise and meaningful here, because here we are checking if this primitive type by the typeText is undefined, this is actually my original thoughts.
Both looks good to me, so @lpinca if you have an objection about undefined, I will change to follow you :)
There was a problem hiding this comment.
No objection, I just find it easier to read without the undefined check but either way LGTM.
|
I’m not sure if this needs a full CI, but here it is: https://ci.nodejs.org/job/node-test-commit/5266/ |
| typeUrl = jsPrimitiveUrl + '#' + typeText + '_type'; | ||
| const primitive = jsPrimitives[typeText]; | ||
| if (primitive !== undefined) { | ||
| typeUrl = jsPrimitiveUrl + '#' + primitive + '_type'; |
There was a problem hiding this comment.
I wonder if it's better to use a template literal here.
|
Done with template string @lpinca Thank you |
We documented most types as Integer, but we don't have link for that. PR-URL: nodejs#8740 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
yorkie
force-pushed
the
improve/docs
branch
from
73a1496
to
ee417c5
Compare
We documented most types as Integer, but we don't have link for that. PR-URL: #8740 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed at eaa0806 because the CI failure seems not related, correct me if I'm wrong :) |
We documented most types as Integer, but we don't have link for that. PR-URL: #8740 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
We documented most types as Integer, but we don't have link for that. PR-URL: nodejs#8740 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
We documented most types as Integer, but we don't have link for that. PR-URL: #8740 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Checklist
Affected core subsystem(s)
doc, tool
Description of change
There are most types like
IntegerandTypedArrayin our documentations, but we don't have a good reference to them like others, so fix them in our doctool, but I'm not sure if we should mapIntegeras aNumbertype, actually even theIntegeris not a real type in v8, by the way, theInt32might be more proper as v8 has the typeInt32Array:)