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.

Sign up for GitHub

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

Jump to bottom

tools: remove redundant code in doc/html.js #20514

Closed
wants to merge 1 commit into from
Closed

tools: remove redundant code in doc/html.js #20514

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented May 4, 2018 •
edited
Loading

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

Status quo

Currently, parseLists() function in tools/doc/html.js has some flaws:

  1. It tries to add <div class="signature">...</div> elements around lists of signature types (function parameters, returned values, property types). However, we have no such class in our api_assets\style.css, so this wrapping list blocks in DIV blocks is redundant.
  2. Due to a logic error, only lists without YAML blocks between heading and list are wrapped. This produces mostly false-positive and false-negative results (most signature lists are not wrapped, many non-signature lists are wrapped).
  3. Logic scaffolding for this wrapping overcomplicates this function.
  4. Processing of YAML blocks depends on unneeded restriction: only YAML blocks straight after heading are processed (currently, it seems we have no other ones, but we may have others in future).

Fixing strategy

  1. Remove redundant DIV wrapping for lists.
  2. Remove connected logic scaffolding.
  3. Process all YAML blocks regardless of their place.
  4. Simplify overall logic for remaining function code.
  5. Reorder conditional blocks to reflect processed source structure.
  6. Update comment and function name to describe function destination more correctly.
  7. Update test.

This PR reduces code by 40 lines and docs size by ~7.5 KB. Only <div class="signature">...</div> wrappers are removed from docs, no other changes are found in results.

The visible styles of wrapped and unwrapped lists are identical:

s1

s2

cc @nodejs/documentation

@vsemozhetbyt vsemozhetbyt added the addons Issues and PRs related to native addons. label May 4, 2018
@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 4, 2018
@vsemozhetbyt vsemozhetbyt added fast-track PRs that do not need to wait for 48 hours to land. and removed addons Issues and PRs related to native addons. labels May 4, 2018
@vsemozhetbyt
Copy link
Contributor Author

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt
Copy link
Contributor Author

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/667/

BridgeAR
BridgeAR approved these changes May 5, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@BridgeAR
Copy link
Member

BridgeAR commented May 5, 2018

@vsemozhetbyt I am really glad you look into these things and improve our docs tooling! 👍

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
@vsemozhetbyt
Copy link
Contributor Author

@BridgeAR Thank you!

tniessen
tniessen approved these changes May 5, 2018
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Glad to see changes which reduce the size of our codebase!

trivikr
trivikr approved these changes May 5, 2018
View reviewed changes
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

@apapirovski
Copy link
Member

Landed in 974df9c. Thanks @vsemozhetbyt!

@apapirovski apapirovski closed this May 7, 2018
apapirovski pushed a commit that referenced this pull request May 7, 2018
@vsemozhetbyt @apapirovski
tools: remove redundant code in doc/html.js
974df9c 
This PR reduces code by 40 lines and docs size by ~7.5 KB. Only
<div class="signature">...</div> wrappers are removed from docs,
no other changes are found in results.

PR-URL: #20514
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt vsemozhetbyt deleted the tools-doc-html-remove-unneeded branch May 7, 2018 09:59
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@vsemozhetbyt @MylesBorins
tools: remove redundant code in doc/html.js
a068808 
This PR reduces code by 40 lines and docs size by ~7.5 KB. Only
<div class="signature">...</div> wrappers are removed from docs,
no other changes are found in results.

PR-URL: #20514
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@vsemozhetbyt @MylesBorins
tools: remove redundant code in doc/html.js
b215f92 
This PR reduces code by 40 lines and docs size by ~7.5 KB. Only
<div class="signature">...</div> wrappers are removed from docs,
no other changes are found in results.

PR-URL: #20514
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt vsemozhetbyt mentioned this pull request May 8, 2018
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request May 9, 2018
@vsemozhetbyt @MylesBorins
tools: remove redundant code in doc/html.js
a4d2089 
This PR reduces code by 40 lines and docs size by ~7.5 KB. Only
<div class="signature">...</div> wrappers are removed from docs,
no other changes are found in results.

PR-URL: #20514
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

Assignees
No one assigned
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. fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vsemozhetbyt @BridgeAR @apapirovski @tniessen @trivikr @nodejs-github-bot