Skip to content

Brace style linting - #7630

Closed
Trott wants to merge 4 commits into
nodejs:masterfrom
Trott:brace-style
Closed

Brace style linting#7630
Trott wants to merge 4 commits into
nodejs:masterfrom
Trott:brace-style

Conversation

@Trott

@TrottTrott commented Jul 9, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tools lib benchmark test

Description of change

Enable brace-style in ESLint.

Ref: #7094 (comment)

/cc @trevnorris@silverwind

Trott added 4 commits July 8, 2016 17:21
This change is in preparation for lint-enforced brace style.
This change is in preparation for a lint rule to enforce brace style.
This change is in preparation for lint enforcement of brace style.
@TrottTrott added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 9, 2016
@nodejs-github-botnodejs-github-bot added module Issues and PRs related to the module subsystem. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Jul 9, 2016
@Trott

Trott commented Jul 9, 2016

Copy link
Copy Markdown
MemberAuthor

@mscdex

Copy link
Copy Markdown
Contributor

LGTM

@Trott

Trott commented Jul 9, 2016

Copy link
Copy Markdown
MemberAuthor

Only failure on CI is a build failure on a Raspberry Pi device.

@targos

Copy link
Copy Markdown
Member

LGTM

1 similar comment
@bnoordhuis

Copy link
Copy Markdown
Member

LGTM

@Fishrock123Fishrock123 removed module Issues and PRs related to the module subsystem. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Jul 9, 2016
return function() {
return stream[method].apply(stream, arguments);
};
}(i);

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.

Unrelated to this change, but I think one level of function wrapping can be removed here:

this[i]=function(method){returnstream[method].apply(stream,arguments);}(i);

@silverwind

Copy link
Copy Markdown
Contributor

LGTM

@Trott

Copy link
Copy Markdown
MemberAuthor

@Fishrock123 Should I treat your "confused" reaction merely as an expression of mild disappointment? Or should I treat it as a "-1, do not do this without successfully persuading me first that this is A Good Thing"?

(Either way, would I be correct to guess that your concerns are around churn / whitespace-only changes?)

@trevnorris

Copy link
Copy Markdown
Contributor

Much thanks for taking care of this. LGTM.

@Trott

Copy link
Copy Markdown
MemberAuthor

I'll land this after another 12 hours or so unless someone objects.

@rvagg

Copy link
Copy Markdown
Member

🎆 lgtm

Trott added a commit to Trott/io.js that referenced this pull request Jul 12, 2016
This change is in preparation for lint-enforced brace style.
PR-URL: nodejs#7630
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Trott added a commit to Trott/io.js that referenced this pull request Jul 12, 2016
Enable `brace-style` in ESLint.
Ref: nodejs#7094 (comment)
PR-URL: nodejs#7630
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@Trott

Copy link
Copy Markdown
MemberAuthor

Landed in 5b63d48 and 863952e

@TrottTrott closed this Jul 12, 2016
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
This change is in preparation for lint-enforced brace style.
PR-URL: #7630
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
Enable `brace-style` in ESLint.
Ref: #7094 (comment)
PR-URL: #7630
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@evanlucasevanlucas mentioned this pull request Jul 18, 2016
@MylesBorins

Copy link
Copy Markdown
Contributor

@Trott would you be willing to backport?

@Trott

Copy link
Copy Markdown
MemberAuthor

@thealphanerd Backported in #8348

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.lib / srcIssues and PRs related to general changes in the lib or src directory.testIssues and PRs related to the tests.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@Trott@mscdex@targos@bnoordhuis@silverwind@trevnorris@rvagg@MylesBorins@Fishrock123@nodejs-github-bot