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

Brace style linting #7630

Closed
wants to merge 4 commits into from
Closed

Brace style linting #7630

wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 9, 2016

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.
@Trott Trott 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-bot nodejs-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
Copy link
Member Author

Trott commented Jul 9, 2016

@mscdex
Copy link
Contributor

mscdex commented Jul 9, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Jul 9, 2016

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

@targos
Copy link
Member

targos commented Jul 9, 2016

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

@Fishrock123 Fishrock123 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
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) {
  return stream[method].apply(stream, arguments);
}(i);

@silverwind
Copy link
Contributor

LGTM

@Trott
Copy link
Member Author

Trott commented Jul 11, 2016

@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
Contributor

Much thanks for taking care of this. LGTM.

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

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

@rvagg
Copy link
Member

rvagg commented Jul 12, 2016

🎆 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
Member Author

Trott commented Jul 12, 2016

Landed in 5b63d48 and 863952e

@Trott Trott 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>
@MylesBorins
Copy link
Contributor

@Trott would you be willing to backport?

@Trott
Copy link
Member Author

Trott commented Aug 31, 2016

@thealphanerd Backported in #8348

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. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants