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,benchmark: increase lint compliance #5429

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 25, 2016

In the hopes of soon having the benchmark code linted, this change
groups all the likely non-controversial lint-compliance changes such as
indentation, semi-colon usage, and single-vs.-double quotation marks.

Other lint rules may have subtle performance implications in the V8
currently shipped with Node.js. Those changes will require more careful
review and will be in a separate change.

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Feb 25, 2016
@Trott Trott changed the title tool,benchmark: increase lint compliance tools,benchmark: increase lint compliance Feb 25, 2016
@silverwind
Copy link
Contributor

LGTM

@Trott
Copy link
Member Author

Trott commented Feb 27, 2016

@nodejs/benchmarking @mscdex

In the hopes of soon having the benchmark code linted, this change
groups all the likely non-controversial lint-compliance changes such as
indentation, semi-colon usage, and single-vs.-double quotation marks.

Other lint rules may have subtle performance implications in the V8
currently shipped with Node.js. Those changes will require more careful
review and will be in a separate change.
@Trott
Copy link
Member Author

Trott commented Feb 27, 2016

" buff." + fn + "(0, " + JSON.stringify(noAssert) + ");",
"}"
].join("\n"));
'for (var i = 0; i !== ' + len + '; i++) {',
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we use template strings for this kind of thing now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mscdex I wanted to be conservative in the change set to keep the diff manageable. And I wanted to avoid anything that might foster any discussion whatsoever about potential performance issues. If you'd prefer I go through and convert to template strings where it seems to make sense, I can do that. If left to my own devices, I'd prefer to either not do it or put that in as a separate independent pull request.

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Feb 28, 2016
In the hopes of soon having the benchmark code linted, this change
groups all the likely non-controversial lint-compliance changes such as
indentation, semi-colon usage, and single-vs.-double quotation marks.

Other lint rules may have subtle performance implications in the V8
currently shipped with Node.js. Those changes will require more careful
review and will be in a separate change.

PR-URL: nodejs#5429
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott
Copy link
Member Author

Trott commented Feb 28, 2016

Landed in dcfda10

@Trott Trott closed this Feb 28, 2016
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
In the hopes of soon having the benchmark code linted, this change
groups all the likely non-controversial lint-compliance changes such as
indentation, semi-colon usage, and single-vs.-double quotation marks.

Other lint rules may have subtle performance implications in the V8
currently shipped with Node.js. Those changes will require more careful
review and will be in a separate change.

PR-URL: #5429
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins
Copy link
Contributor

@Trott likely this should be merged in with the various benchmark changes you have been doing for a single backport PR

@Trott
Copy link
Member Author

Trott commented Mar 17, 2016

@thealphanerd If it's all the same to you, I think it would be easier to do them as one-to-one mappings to backport PRs. This one maps to #5770

@MylesBorins
Copy link
Contributor

works for me... thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants