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

Replaced string concats with template literals #15858

Closed

Conversation

Ethan-Arrowood
Copy link
Contributor

Replaced string concatenation in tools/lint-js.js with template literals.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 6, 2017
tools/lint-js.js Outdated
};

// Check if we should fix errors that are fixable
if (process.argv.indexOf('-F') !== -1)
cliOptions.fix = true;
if (process.argv.indexOf('-F') !== -1) cliOptions.fix = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 I think these kinds of changes make things less readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize I think prettier made these changes automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. There are other instances of this in this file that should also be reverted.

tools/lint-js.js Outdated

// Calculate and format the data for displaying
const elapsed = process.hrtime(startTime)[0];
const mins = padString(Math.floor(elapsed / 60), 2, '0');
const secs = padString(elapsed % 60, 2, '0');
const passed = padString(successes, 6, ' ');
const failed = padString(failures, 6, ' ');
var pct = Math.ceil(((totalPaths - paths.length) / totalPaths) * 100);
var pct = Math.ceil((totalPaths - paths.length) / totalPaths * 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the added parentheses so the order of operations is immediately clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another prettier auto-change. My mistake.

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@Ethan-Arrowood
Copy link
Contributor Author

@mscdex I've gone back through and reverted all Prettier edits

tools/lint-js.js Outdated
@@ -139,8 +139,7 @@ if (cluster.isMaster) {
printProgress();
outFn('\r\n');
}
if (code === 0)
process.exit(failures ? 1 : 0);
if (code === 0) process.exit(failures ? 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

One here yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About to board my plane so other edits will have to wait until I get home (unless my plane has internet).

@Trott
Copy link
Member

Trott commented Oct 9, 2017

@Ethan-Arrowood
Copy link
Contributor Author

@Trott it seems like it is failing on node-test-commit-osx. Any idea on how I could remedy this?

@Trott
Copy link
Member

Trott commented Oct 10, 2017

@Trott it seems like it is failing on node-test-commit-osx. Any idea on how I could remedy this?

@Ethan-Arrowood That looks like a Ci infrastructure glitch and not anything to do with the changes here. I don't think you need to do anything about it.

@Trott
Copy link
Member

Trott commented Oct 10, 2017

Re-running CI on OS X only: https://ci.nodejs.org/job/node-test-commit-osx/12970/

@Trott Trott self-assigned this Oct 10, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 10, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: nodejs#15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 10, 2017

Landed in 27b5bf1.
Thanks for the contribution! 🎉

@Trott Trott closed this Oct 10, 2017
@Trott Trott removed their assignment Oct 10, 2017
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: #15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 11, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: nodejs/node#15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: #15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 18, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: #15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Replace string concatenation in `tools/lint-js.js` with template
literals.

PR-URL: #15858
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants