Skip to content
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

tools: overhaul tools/doc/html.js #20613

Closed
wants to merge 2 commits into from
Closed

tools: overhaul tools/doc/html.js #20613

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented May 8, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

I was trying to facilitate reviewing this PR as far as I could, singling out changes that refactor monolithic structures or alter doc results (see #20307, #20514, #20552, #20586).

However, this PR still produces big and complicated diff as it has many small changes scattered all over the script and also bears refactoring (simplifying, merging, inlining etc) closely coupled but tracery systems. It would be hard to split these changes into dozens of separate PRs.

To make review even possible, I've split it into 2 commits: local changes + pure reordering of major blocks with no additions/deletions. Please, review these commits separately.

commit 1

  • Modernize:

    • Replace var with const / let.
    • Replace common functions with arrow functions.
    • Use destructuring.
    • Shorthand object literals.
  • Optimize:

    • Reduce global variables.
    • Reduce intermediate structures, variables or assignments.
    • Reduce function calls.
    • Inline small one-off functions (sometimes removing wrong abstractions).
    • Join functions with similar purpose (as toHTML() and render()) to reduce data transferring.
    • Remove conditions that cannot be false.
    • Simplify RegExps.
    • Move RegExp declarations out of hot functions.
    • Replace RegExp with string when string suffices, use .includes().
    • Replace .match() with .test() in boolean context.
    • Replace ['str', 'str'].join() with straight string concatenation.
    • Replace new Array(length + 1).join(str) with str.repeat(length).
    • Replace loops with appropriate array methods.
    • De-callbackify synchronous function chains.
  • Clarify:

    • Correct comments.
    • Sync comments with the actual source state.
    • Remove redundant or obsolete comments.
    • Expand some one/two-letters variable names.
    • Rename confusingly similar variables.
    • Rename functions to define their aim more correctly.

Commit 2

  • Reorder function declarations chronologically (in order of calls) to make reading script code easier.

This PR produces HTML docs identical to the previous ones, reducing the script by ~ 100 lines (with previous 4 PRs, the size is reduced by 175 lines) and I hope the script becomes a bit clearer and simpler.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 8, 2018
@vsemozhetbyt vsemozhetbyt changed the title Tools doc html.js overhaul tools: overhaul tools/doc/html.js May 8, 2018
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

сс @nodejs/documentation

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2018
vsemozhetbyt added a commit that referenced this pull request May 14, 2018
PR-URL: #20613
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt
Copy link
Contributor Author

Landed in 12b0159
Thank you for the review.

@vsemozhetbyt vsemozhetbyt deleted the tools-doc-html.js-overhaul branch May 14, 2018 16:08
addaleax pushed a commit that referenced this pull request May 14, 2018
PR-URL: #20613
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax addaleax mentioned this pull request May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants