tools: restore change of doc building function signatures to opts hashes #6690
Conversation
These signatures were originally converted to opts hashes in nodejs#3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in nodejs#6680.
mscdex
added
doc
|
Looks like the CI failure is in |
| */ | ||
| function toHTML(opts, cb) { | ||
| var template = opts.template; | ||
| var nodeVersion = opts.nodeVersion || process.version; |
There was a problem hiding this comment.
Thanks for the feedback @addaleax. Yes, they could be. FWIW when I first PRed those changes there was no const in that file and after the changes were first merged in #3888 and reverted in #6680 I didn't want to rock the boat with anything other than changes that had already been previously +1ed (since the original PR had a hard enough time landing). Also FWIW there's a bunch of other var in the file that could be const. The specific vars you commented on are each only referenced once, so honestly probably the best thing is to just eliminate those bindings and use opts where those are referenced. The same probably goes for at least some of the vars in render().
Let me know if you want me to make a change either way.
|
@jmm Yes, the CI failure is unrelated. This changes only the function signatures, not the behaviour in any way, right? |
|
@jmm A good rule of thumb is that I’d actually like it if you could make these changes (or remote the |
|
LGTM. If no objections, we could go ahead merging this any way. I am suggesting pushing around code a lot in #6974 and can amend any nits there if someone likes me to. |
|
Sounds good, too. @eljefedelrodeodeljefe You wanna do the merge? ;) |
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
|
Thanks. Landed in c1ffb9f |
Ok, thanks. I mentioned the others in case it'd be preferred to update them all in one go, but that makes sense, I'll try to keep that in mind.
If it were up to me I'd probably eliminate some (maybe all) of those bindings at this point -- and perhaps I should've done it that way in the first place. If someone wants me to update anything there, ping me, but it sounds like others are planning to do it along with other changes. Either way is fine by me. Thanks everyone! |
|
@jmm I guess the rest can be done, as @eljefedelrodeodeljefe said above, in #6974 if that one lands, or basically whenever the next chance for that occurs. Feel free to weigh in over there! |
These signatures were originally converted to opts hashes in nodejs#3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in nodejs#6680. PR-URL: nodejs#6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
|
Thanks @addaleax. I see that discussion has moved to #6999 -- I'll try to check that out sometime. (I just skimmed it real quick and saw your note about keeping it v4 compatible for the |
|
should this be backported? |
|
ping @nodejs/documentation |
These signatures were originally converted to opts hashes in nodejs#3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in nodejs#6680. PR-URL: nodejs#6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
These signatures were originally converted to opts hashes in #3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in #6680. PR-URL: #6690 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Checklist
Affected core subsystem(s)
tools
Description of change
/cc @thealphanerd In #3888 signatures of some functions used to build the docs were converted to take options hashes and then those changes were interpreted as the intrinsic cause of a test failure and reverted in #6680. The test did fail because the callsite signatures didn't match the updates, but passes once that's resolved. This PR restores the changes converting those signatures to take options hashes, and updates the callsites in the recently added
test/doctool/test-doctool-html.js. See discussion in #6680 for more info.make testpasses for me locally, and anecdotal testing ofNODE=node make doc-only(what #3888 enabled) works as expected.Node version defaults
I retained the additions from #6680 that set a default node version:
However, I'd note that those additions aren't covered by tests and that of the 3 of them, I'd consider 2 of them to be redundant with the one in
tools/doc/html.js:toHTML.ES6
I noticed that since I started #3888 some ES6 (template literals) were added to
tools/doc/html.js. Some of the changes I made could be written considerably more elegantly using some ES6 (a little bit with object literal shorthand properties, and most of all with destructuring), but the purpose of #3888 was to make it possible to build the docs (which relies on the files in this PR) using an existing Node install, which may be earlier than the Node version for which the docs are being built. So it'd be worth keeping that in mind and perhaps limiting the features used in the doc build tools to those that'll work a few versions back. (Example: I think template literals and object literal shorthand properties are both available without a flag in v4, but not destructuring.)