Issue 4993 - Apply operator-linebreak ESLint rule consistently

Issue 4993 - Apply operator-linebreak ESLint rule consistently

See the discussion here https://codereview.adblockplus.org/29374674/diff/29383722/lib/devtools.js#newcode89 . It turns out the default configuration for this rule is:

  "after", { "overrides": { "?": "before", ":": "before" } }

Which makes a special exception for the `?` and `:` operators.

Note: After means "linebreaks to be placed after the operator", which is kind of misleading.

[kzar, 2017-03-15 04:13:11]

Patch Set 1

[Wladimir Palant, 2017-03-15 15:12:04]

LGTM

[Felix Dahlke, 2017-03-16 11:24:39]

On 2017/03/15 15:12:04, Wladimir Palant wrote:
> LGTM

NOT LGTM - the Mozilla coding style actually states:

"Overlong expressions not joined by && and || should break so the operator
starts on the second line and starts in the same column as the start of the
expression in the first line. This applies to ?:, binary arithmetic operators
including +, and member-of operators (in particular the . operator in
JavaScript, see the Rationale)."

That's how I've always done it IIRC. If we want to change that, we should
overrule it in our coding style. (But I actually prefer it the Mozilla way.)

[kzar, 2017-03-16 12:01:04]

On 2017/03/16 11:24:39, Felix Dahlke wrote:
> On 2017/03/15 15:12:04, Wladimir Palant wrote:
> > LGTM
> 
> NOT LGTM - the Mozilla coding style actually states:
> 
> "Overlong expressions not joined by && and || should break so the operator
> starts on the second line and starts in the same column as the start of the
> expression in the first line. This applies to ?:, binary arithmetic operators
> including +, and member-of operators (in particular the . operator in
> JavaScript, see the Rationale)."
> 
> That's how I've always done it IIRC. If we want to change that, we should
> overrule it in our coding style. (But I actually prefer it the Mozilla way.)

Interesting, well spotted. I guess by that logic the ESLint rule should really
be this:

   "operator-linebreak": ["error", "before", { "overrides": { "||": "after",
"&&": "after" } }]

Thing is that causes a whole bunch of ESLint errors, I guess our code didn't
really follow Mozilla's rule so far. I wonder what's better, changing our style
guide to override what Mozilla's rule, or changing our ESLint congfiguration and
adblockplus{core,chrome,ui} to follow.

[Felix Dahlke, 2017-03-16 12:12:23]

On 2017/03/16 12:01:04, kzar wrote:
> On 2017/03/16 11:24:39, Felix Dahlke wrote:
> > On 2017/03/15 15:12:04, Wladimir Palant wrote:
> > > LGTM
> > 
> > NOT LGTM - the Mozilla coding style actually states:
> > 
> > "Overlong expressions not joined by && and || should break so the operator
> > starts on the second line and starts in the same column as the start of the
> > expression in the first line. This applies to ?:, binary arithmetic
operators
> > including +, and member-of operators (in particular the . operator in
> > JavaScript, see the Rationale)."
> > 
> > That's how I've always done it IIRC. If we want to change that, we should
> > overrule it in our coding style. (But I actually prefer it the Mozilla way.)
> 
> Interesting, well spotted. I guess by that logic the ESLint rule should really
> be this:
> 
>    "operator-linebreak": ["error", "before", { "overrides": { "||": "after",
> "&&": "after" } }]
> 
> Thing is that causes a whole bunch of ESLint errors, I guess our code didn't
> really follow Mozilla's rule so far. I wonder what's better, changing our
style
> guide to override what Mozilla's rule, or changing our ESLint congfiguration
and
> adblockplus{core,chrome,ui} to follow.

Being pragmatic, I'd say this depends on what style is more prevalent. Whatever
we use most of the time, we should probably standardise, Mozilla style or not.

(Personally I do prefer the Mozilla style for aesthetic reasons, but that's not
really a good reason to change a lot of code.)

[Wladimir Palant, 2017-03-16 14:25:12]

On 2017/03/16 11:24:39, Felix Dahlke wrote:
> "Overlong expressions not joined by && and || should break so the operator
> starts on the second line and starts in the same column as the start of the
> expression in the first line. This applies to ?:, binary arithmetic operators
> including +, and member-of operators (in particular the . operator in
> JavaScript, see the Rationale)."

Interesting, I wasn't aware of that. From what I know, we've been matching
Mozilla's style as far as "." goes - but otherwise we keep operators before the
line break pretty consistently. But ESLint should be able to show which style is
really prevalent.

[kzar, 2017-03-17 10:21:01]

On 2017/03/16 14:25:12, Wladimir Palant wrote:
> From what I know, we've been matching
> Mozilla's style as far as "." goes - but otherwise we keep operators before
the
> line break pretty consistently.

Yep, that was my understanding too.

> But ESLint should be able to show which style is really prevalent.

Good idea. I've tried both rules against the three repositories before any
ESLint related changes were made to them. Here are the results:

  "operator-linebreak": ["error", "before", { "overrides": { "||": "after",
"&&": "after" } }],
  adblockpluschrome 92
  adblockpluscore 45
  adblockplusui 13

  "operator-linebreak": ["error", "after"],
  adblockpluschrome 7
  adblockpluscore 1
  adblockplusui 6

So ["error", "after"] is the clear winner there. I will open a review to correct
our coding style guide.

[Felix Dahlke, 2017-03-17 11:01:03]

LGTM then.