tools: restore change of doc building function signatures to opts hashes by jmm · Pull Request #6690 · nodejs/node · GitHub

Checklist

tests and code linting passes
the commit message follows commit guidelines

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 test passes for me locally, and anecdotal testing of NODE=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:

nodeVersion = nodeVersion || process.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.)

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.

ci: https://ci.nodejs.org/job/node-test-pull-request/2587/

Looks like the CI failure is in sequential/test-next-tick-error-spin.js in one build and unrelated to this PR?

I think these can be const

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?

@addaleax Ok, thanks for confirming. That is correct. Originally these changes were PR'ed along with a behavior change (accepting a --node-version opt instead of using process.version) in #3888, but this PR is not intended to change any behavior, only the signatures and the call sites.

@jmm A good rule of thumb is that var -> const replacements can take place in those lines of a PR where the code’s being touched anyway so it doesn’t make running git blame & co harder.

I’d actually like it if you could make these changes (or remote the var = … as you suggested in favour of accessing the parameters object directly), but this LGTM either way. And don’t worry, I’d like to land it before #6943 so I can make sure there are no conflicts (or the burden of resolving them doesn’t fall back onto you).

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 <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

Thanks. Landed in c1ffb9f

@addaleax

A good rule of thumb is that var -> const replacements can take place in those lines of a PR where the code’s being touched anyway so it doesn’t make running git blame & co harder.

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.

I’d actually like it if you could make these changes (or remote the var = … as you suggested in favour of accessing the parameters object directly), but this LGTM either way. And don’t worry, I’d like to land it before #6943 so I can make sure there are no conflicts (or the burden of resolving them doesn’t fall back onto you).

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 <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

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 doc-only target -- thanks!) Not sure where that leaves the var|const|opts issue, but if someone wants me to make a change WRT that feel free to ping me.

@jmm We will! In the meantime, there’s not much “discussion” going on in #6999 yet, but if you have an opinion on the path forward, you should feel free to share that.

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 <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>

tools: restore change of signatures to opts hashes

76577aa

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 Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 11, 2016

addaleax reviewed May 24, 2016
View reviewed changes

addaleax mentioned this pull request May 25, 2016

doc, tools: refactor html generation #6974

Closed

4 tasks

eljefedelrodeodeljefe closed this May 25, 2016

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 12, 2016

addaleax mentioned this pull request Jul 12, 2016

Bring the doctool up to date on v4.x #7668

Merged

addaleax removed the lts-watch-v4.x label Jul 12, 2016

MylesBorins added the dont-land-on-v4.x label Jul 12, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

+               */
+              function toHTML(opts, cb) {
+                var template = opts.template;
+                var nodeVersion = opts.nodeVersion || process.version;

tools: restore change of doc building function signatures to opts hashes by jmm · Pull Request #6690 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: restore change of doc building function signatures to opts hashes - #6690

tools: restore change of doc building function signatures to opts hashes#6690jmm wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jmm:build-docs-signaturesjmm/node:build-docs-signaturesCopy head branch name to clipboard

Conversation

jmm commented May 11, 2016

Checklist

Affected core subsystem(s)

Description of change

Node version defaults

ES6

Uh oh!

MylesBorins commented May 11, 2016

Uh oh!

jmm commented May 12, 2016

Uh oh!

addaleax May 24, 2016

Choose a reason for hiding this comment

Uh oh!

jmm May 25, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 24, 2016

Uh oh!

jmm commented May 25, 2016

Uh oh!

addaleax commented May 25, 2016

Uh oh!

eljefedelrodeodeljefe commented May 25, 2016

Uh oh!

addaleax commented May 25, 2016

Uh oh!

eljefedelrodeodeljefe commented May 25, 2016

Uh oh!

jmm commented May 26, 2016

Uh oh!

addaleax commented May 26, 2016

Uh oh!

jmm commented Jun 3, 2016

Uh oh!

addaleax commented Jun 3, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

MylesBorins commented Jul 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tools: restore change of doc building function signatures to opts hashes#6690
jmm wants to merge 1 commit into
nodejs:masterfrom
jmm:build-docs-signatures