FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Brace style linting by Trott · Pull Request #7630 · nodejs/node · GitHub

/ node Public

Brace style linting - #7630

Closed
Trott wants to merge 4 commits into
nodejs:masterfrom
Trott:brace-style
ConversationCommits (4)ChecksFiles changed
Closed

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

Conversation

Trott 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
lib: implement consistent brace style
a3480b2 
This change is in preparation for lint-enforced brace style.
benchmark: implement consistent brace style
cd9d871 
This change is in preparation for a lint rule to enforce brace style.
test: implement consistent braces
233c22f 
This change is in preparation for lint enforcement of brace style.
tools: enforce JS brace style with linting
ad9ea14 
Enable `brace-style` in ESLint.

Ref: nodejs#7094 (comment)
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 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 commented Jul 9, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/3224/

mscdex commented Jul 9, 2016

Copy link
Copy Markdown
Contributor

LGTM

Trott commented Jul 9, 2016

Copy link
Copy Markdown
Member Author

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

targos commented Jul 9, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

bnoordhuis commented Jul 9, 2016

Copy link
Copy Markdown
Member

LGTM

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
silverwind reviewed Jul 9, 2016
View reviewed changes
Comment thread lib/_stream_readable.js
return function() {
return stream[method].apply(stream, arguments);
};
}(i);

silverwind Jul 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 9, 2016

Copy link
Copy Markdown
Contributor

LGTM

Trott commented Jul 11, 2016

Copy link
Copy Markdown
Member Author

@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 commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

Much thanks for taking care of this. LGTM.

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

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

rvagg commented Jul 12, 2016

Copy link
Copy Markdown
Member

🎆 lgtm

Trott added a commit to Trott/io.js that referenced this pull request Jul 12, 2016
lib,benchmark,test: implement consistent braces
5b63d48 
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
tools: enforce JS brace style with linting
863952e 
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 commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

Landed in 5b63d48 and 863952e

Trott closed this Jul 12, 2016
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
lib,benchmark,test: implement consistent braces
fc0ed2e 
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
tools: enforce JS brace style with linting
a82573d 
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>
evanlucas mentioned this pull request Jul 18, 2016
Merged
AndreasMadsen mentioned this pull request Jul 22, 2016
Merged
4 tasks

MylesBorins commented Aug 30, 2016

Copy link
Copy Markdown
Contributor

@Trott would you be willing to backport?

Trott commented Aug 31, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd Backported in #8348

Trott mentioned this pull request Aug 31, 2016
Closed
Trott deleted the brace-style branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL