tools: enforce consistent operator linebreak style #10178

targos · 2016-12-08T09:43:08Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib, test, tools

Description of change

Commit 1:

lib,test: use consistent operator linebreak style

We have a tacit rule that for multiline statements, the operator should
be placed before the linebreak. This commit commit fixes the few
violations of this rule in the code base.
This allows us to enable the corresponding ESLint rule.

Commit 2:

tools: enforce consistent operator linebreak style

Adds the `operator-linebreak` rule to our ESLint config.

I disabled the check for the ternary operators because ESLint has a special case for it (rule is inverted) and it's not obvious what we want. There are about 30 violations for each case (linebreak before or after the operator).
I personally would like it to be after, like everything else.

bnoordhuis · 2016-12-08T12:21:33Z

There are about 30 violations for each case (linebreak before or after the operator).

I always put the linebreak after the ? because you miss context if you are grepping through code and the operator is on the next line.

Fishrock123

¯\_(ツ)_/¯

mhdawson

LGTM

targos · 2016-12-08T17:53:48Z

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

Trott

LGTM

not-an-aardvark · 2016-12-08T23:39:42Z

I disabled the check for the ternary operators because ESLint has a special case for it (rule is inverted) and it's not obvious what we want.

Actually, ESLint doesn't have a special case for those operators, but it looks like ESLint's docs are currently a bit misleading on that point. See eslint/eslint#7726 for an explanation of the actual behavior. (tl;dr: If we just use operator-linebreak: [2, after], the rule will be enforced as expected for all operators without inverting ternary operators.)

targos · 2016-12-09T07:37:29Z

@not-an-aardvark

If we just use operator-linebreak: [2, after], the rule will be enforced as expected for all operators without inverting ternary operators.)

I figured this out while playing with the options. What I meant is that if ESLint's default behavior is to give a special treatment to ternary operators, maybe people here feel the same about it.

thefourtheye

LGTM if CI is happy.

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

targos · 2016-12-10T09:52:11Z

Landed in 0cd1f54...7c2dbd1

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: #10178 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

targos added 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. labels Dec 8, 2016

targos requested a review from Trott December 8, 2016 09:43

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 8, 2016

targos removed the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 8, 2016

bnoordhuis approved these changes Dec 8, 2016

View reviewed changes

silverwind approved these changes Dec 8, 2016

View reviewed changes

cjihrig approved these changes Dec 8, 2016

View reviewed changes

Fishrock123 approved these changes Dec 8, 2016

View reviewed changes

mhdawson approved these changes Dec 8, 2016

View reviewed changes

Trott approved these changes Dec 8, 2016

View reviewed changes

not-an-aardvark approved these changes Dec 8, 2016

View reviewed changes

thefourtheye approved these changes Dec 9, 2016

View reviewed changes

targos added 2 commits December 10, 2016 10:46

targos added lts-watch-v4.x labels Dec 10, 2016

targos force-pushed the eslint-operator-linebreak branch from 1a7952b to 7c2dbd1 Compare December 10, 2016 09:50

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

targos closed this Dec 10, 2016

targos deleted the eslint-operator-linebreak branch December 10, 2016 09:52

targos merged commit 7c2dbd1 into nodejs:master Dec 10, 2016

targos removed the dont-land-on-v7.x label Dec 10, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

targos added land-on-v4.x and removed lts-watch-v4.x labels Dec 28, 2016

MylesBorins added the dont-land-on-v4.x label Jan 16, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 16, 2017

targos removed the dont-land-on-v4.x label Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: enforce consistent operator linebreak style #10178

tools: enforce consistent operator linebreak style #10178

targos commented Dec 8, 2016 •

edited

Loading

bnoordhuis commented Dec 8, 2016

Fishrock123 left a comment

mhdawson left a comment

targos commented Dec 8, 2016

Trott left a comment

not-an-aardvark commented Dec 8, 2016

targos commented Dec 9, 2016

thefourtheye left a comment

targos commented Dec 10, 2016

tools: enforce consistent operator linebreak style #10178

tools: enforce consistent operator linebreak style #10178

Conversation

targos commented Dec 8, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

bnoordhuis commented Dec 8, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

targos commented Dec 8, 2016

Trott left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Dec 8, 2016

targos commented Dec 9, 2016

thefourtheye left a comment

Choose a reason for hiding this comment

targos commented Dec 10, 2016

targos commented Dec 8, 2016 •

edited

Loading