-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
4378e75
to
bca860d
Compare
LGTM |
@nodejs/benchmarking @mscdex |
bca860d
to
7b5d451
Compare
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.
7b5d451
to
261cae5
Compare
" buff." + fn + "(0, " + JSON.stringify(noAssert) + ");", | ||
"}" | ||
].join("\n")); | ||
'for (var i = 0; i !== ' + len + '; i++) {', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
LGTM |
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>
Landed in dcfda10 |
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>
@Trott likely this should be merged in with the various benchmark changes you have been doing for a single backport PR |
@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 |
works for me... thanks! |
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.