Skip to content

tools: modernize and optimize doc/addon-verify.js#20188

Closed
vsemozhetbyt wants to merge 1 commit intonodejs:masterfrom
vsemozhetbyt:tools-doc-addon-verify
Closed

tools: modernize and optimize doc/addon-verify.js#20188
vsemozhetbyt wants to merge 1 commit intonodejs:masterfrom
vsemozhetbyt:tools-doc-addon-verify

Conversation

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 21, 2018 •
edited
Loading

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

Modernize:

  • Replace var with const / let.
  • Replace common functions with arrow functions.
  • Use destructuring.
  • Use String.prototype.padStart(), String.prototype.endsWith().

Optimize:

  • Reduce function calls.
  • Reduce intermediate variables.
  • Cache retrieved object properties.
  • Move RegExp declaration out of a cycle.
  • Simplify RegExps.
  • Replace RegExp with string when string suffices.
  • Remove conditions that cannot be false.
  • Replace for..in with Object.keys().forEach().

Also, eliminate needlessly complicated function chains:

  • ondone callback only checks errors;
  • if there is an error, it is called once and throws, then script exits;
  • if there are no errors, it is noop;
  • so there is no need to wrap it into once() function;
  • and there is no need to call it without errors;
  • we can eliminate it and replace with throw where an error occurs;
  • we can also replace onprogress callback with console.log in place;
  • at last, we can eliminate waiting counter and once() utility.

The new script produces results identical to the old ones.

@vsemozhetbyt
tools: modernize and optimize doc/addon-verify.js
8510b9d 
Modernize:
* Replace `var` with `const` / `let`.
* Replace common functions with arrow functions.
* Use destructuring.
* Use spread, eliminate `arguments`.
* Use `String.prototype.padStart()`, `String.prototype.endsWith()`.

Optimize:
* Reduce function calls.
* Reduce intermediate variables.
* Cache retrieved object properties.
* Move RegExp declaration out of a cycle.
* Simplify RegExps.
* Replace RegExp with string when string suffices.
* Remove conditions that cannot be false.
* Replace for..in with `Object.keys().forEach()`.
* Rename confusingly similar variables.

Also, eliminate needlessly complicated function chains:
* `ondone` callback only checks errors;
* if there is an error, it is called once and throws, then script exits;
* if there are no errors, it is noop;
* so there is no need to wrap it into `once()` function
* and there is no need to call it without errors;
* we can eliminate it and replace with `throw` where an error occurs;
* we can also replace `onprogress` callback with `console.log` in place;
* at last, we can eliminate `waiting` counter and `once()` utility.

The new script produces results identical to the old ones.
@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 Apr 21, 2018
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2018 •
edited
Loading

Full CI to be on the safe side: https://ci.nodejs.org/job/node-test-pull-request/14412/

Failures seem unrelated, but I'll rerun:
https://ci.nodejs.org/job/node-test-pull-request/14414/

trivikr
trivikr reviewed Apr 21, 2018
View reviewed changes
tools/doc/addon-verify.js

const validNames = /^\/\/\s+(.*\.(?:cc|h|js))[\r\n]/;
tokens.forEach(({ type, text }) => {
if (type === 'heading') {
Copy link
Member

@trivikr trivikr Apr 21, 2018

Choose a reason for hiding this comment

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

Is the check for text required here?

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If I tested correctly, there cannot be empty headings: they are not recognized as headings by marked:

const marked = require('marked ');

const md = `
# heading

text

#

text
`;

console.log(marked.lexer(md));
[ { type: 'heading', depth: 1, text: 'heading' },
  { type: 'paragraph', text: 'text' },
  { type: 'paragraph', text: '#' },
  { type: 'paragraph', text: 'text' },
  links: {} ]

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 21, 2018

Rerun failed jobs are now green:
https://ci.nodejs.org/job/node-test-commit-osx/17996/
https://ci.nodejs.org/job/node-test-commit-aix/14389/

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

vsemozhetbyt commented Apr 24, 2018

Landed in 6946812

vsemozhetbyt added a commit that referenced this pull request Apr 24, 2018
@vsemozhetbyt
tools: modernize and optimize doc/addon-verify.js
6946812 
Modernize:
* Replace `var` with `const` / `let`.
* Replace common functions with arrow functions.
* Use destructuring.
* Use `String.prototype.padStart()`, `String.prototype.endsWith()`.

Optimize:
* Reduce function calls.
* Reduce intermediate variables.
* Cache retrieved object properties.
* Move RegExp declaration out of a cycle.
* Simplify RegExps.
* Replace RegExp with string when string suffices.
* Remove conditions that cannot be false.
* Replace for..in with `Object.keys().forEach()`.

Also, eliminate needlessly complicated function chains:
* `ondone` callback only checks errors;
* if there is an error, it is called once and throws, then script exits;
* if there are no errors, it is noop;
* so there is no need to wrap it into `once()` function
* and there is no need to call it without errors;
* we can eliminate it and replace with `throw` where an error occurs;
* we can also replace `onprogress` callback with `console.log` in place;
* at last, we can eliminate `waiting` counter and `once()` utility.

The new script produces results identical to the old ones.

PR-URL: #20188
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@vsemozhetbyt vsemozhetbyt deleted the tools-doc-addon-verify branch April 24, 2018 07:40
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@vsemozhetbyt @MylesBorins
tools: modernize and optimize doc/addon-verify.js
b5584c4 
Modernize:
* Replace `var` with `const` / `let`.
* Replace common functions with arrow functions.
* Use destructuring.
* Use `String.prototype.padStart()`, `String.prototype.endsWith()`.

Optimize:
* Reduce function calls.
* Reduce intermediate variables.
* Cache retrieved object properties.
* Move RegExp declaration out of a cycle.
* Simplify RegExps.
* Replace RegExp with string when string suffices.
* Remove conditions that cannot be false.
* Replace for..in with `Object.keys().forEach()`.

Also, eliminate needlessly complicated function chains:
* `ondone` callback only checks errors;
* if there is an error, it is called once and throws, then script exits;
* if there are no errors, it is noop;
* so there is no need to wrap it into `once()` function
* and there is no need to call it without errors;
* we can eliminate it and replace with `throw` where an error occurs;
* we can also replace `onprogress` callback with `console.log` in place;
* at last, we can eliminate `waiting` counter and `once()` utility.

The new script produces results identical to the old ones.

PR-URL: #20188
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Aug 17, 2018
@vsemozhetbyt @MylesBorins
tools: modernize and optimize doc/addon-verify.js
ac05c2b 
Modernize:
* Replace `var` with `const` / `let`.
* Replace common functions with arrow functions.
* Use destructuring.
* Use `String.prototype.padStart()`, `String.prototype.endsWith()`.

Optimize:
* Reduce function calls.
* Reduce intermediate variables.
* Cache retrieved object properties.
* Move RegExp declaration out of a cycle.
* Simplify RegExps.
* Replace RegExp with string when string suffices.
* Remove conditions that cannot be false.
* Replace for..in with `Object.keys().forEach()`.

Also, eliminate needlessly complicated function chains:
* `ondone` callback only checks errors;
* if there is an error, it is called once and throws, then script exits;
* if there are no errors, it is noop;
* so there is no need to wrap it into `once()` function
* and there is no need to call it without errors;
* we can eliminate it and replace with `throw` where an error occurs;
* we can also replace `onprogress` callback with `console.log` in place;
* at last, we can eliminate `waiting` counter and `once()` utility.

The new script produces results identical to the old ones.

PR-URL: #20188
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@trivikr trivikr trivikr 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. 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.

4 participants

@vsemozhetbyt @jasnell @trivikr @nodejs-github-bot