Skip to content

Enable linting for benchmarks - #5517

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:simplify
Closed

Enable linting for benchmarks#5517
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:simplify

Conversation

@Trott

@TrottTrott commented Mar 2, 2016

Copy link
Copy Markdown
Member

Refactor benchmarks to conform with existing lint rules. (In the process, a few broken benchmarks were fixed, e.g. the one for domains.) Enable linting for benchmarks.

/cc @nodejs/benchmarking @mscdex

@TrottTrott added benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Mar 2, 2016
@Trott

Trott commented Mar 2, 2016

Copy link
Copy Markdown
MemberAuthor

Comment threadbenchmark/buffers/buffer-indexof.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't having the last parenthesis on the next line be more consistent (and IMHO better looking) with the changes made in benchmark/compare.js?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agreed. Will change and push.

@Trott

Trott commented Mar 2, 2016

Copy link
Copy Markdown
MemberAuthor

Fixup commit pushed based on @mscdex feedback. PTAL.

@mscdex

Copy link
Copy Markdown
Contributor

LGTM if CI is still ok with it

@Trott

Trott commented Mar 2, 2016

Copy link
Copy Markdown
MemberAuthor

@targos

Copy link
Copy Markdown
Member

LGTM

2 similar comments
@evanlucas

Copy link
Copy Markdown
Contributor

LGTM

@jasnell

Copy link
Copy Markdown
Member

LGTM

@jasnell

Copy link
Copy Markdown
Member

Looks like the PR may need a quick rebase and update before landing.

@Trott

Trott commented Mar 2, 2016

Copy link
Copy Markdown
MemberAuthor

@jasnell Rebased!

jasnell pushed a commit that referenced this pull request Mar 3, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 3, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

Copy link
Copy Markdown
Member

Landed in 1bedeeb and 6d22003

@jasnelljasnell closed this Mar 3, 2016
@Fishrock123Fishrock123 mentioned this pull request Mar 7, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

Copy link
Copy Markdown
Contributor

@Trott this one too for benchmarking backport

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Trott@mscdex@targos@evanlucas@jasnell@MylesBorins