Issue 3692 - Add base ESLint configuration

Issue 3692 - Add base ESLint configuration

This is a continuation of Julian's work, see the related codereview here https://codereview.adblockplus.org/29346539/ .

To see what these rules may mean in practice have a look at the required changes to adblockpluschrome so far. https://github.com/kzar/adblockpluschrome/tree/4864-eslint (Note those changes haven't been tested yet and likely are a little rough.)

[kzar, 2017-02-03 09:28:04]

Patch Set 1

[kzar, 2017-02-03 09:31:29]

On 2017/02/03 09:28:04, kzar wrote:
> Patch Set 1

(Also note that ESLint will not be a complete replacement for Nits in
codereviews. Unfortunately some things like indentation were not possible to
lock down as strictly as I hoped without causing false positives. Still IMO it
will be a vast improvement.)

[Wladimir Palant, 2017-02-03 10:24:13]

Sorry if I duplicated some of the previous discussion, I didn't read it all.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:16: "computed-property-spacing": "error",
It's a pity that the consensus was apparently to remove consistent-return, I
consider that rule very useful and an occasional explicit "return undefined" not
a bad thing.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
Is this a good idea? Lexical declarations inside case clauses are disallowed for
a reason - a case clause doesn't have its own lexical scope, only the switch
statement does. So the following is an error because it redeclares variable x:

switch (foo)
{
  case 1:
    let x = 1;
    break;
  case 2:
    let x = 2;
    break;
}

The correct way of writing this requires an additional block:


switch (foo)
{
  case 1:
  {
    let x = 1;
    break;
  }
  case 2:
  {
    let x = 2;
    break;
  }
}

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:31: "no-cond-assign": "off",
Frankly, I'd rather not disable this. An assignment inside a condition is rarely
intended, and even when it is it's bad for readability.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
Why is this rule turned off? Flagging undeclared variables is the most important
feature of a linter. Any variables coming from external scopes should either be
covered by the environment or /* global */ comments.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:71: "prefer-spread": "error",
What about prefer-rest-params?

[Sebastian Noack, 2017-02-03 12:49:32]

> Also note that each project using ESLint will have it's own configuration
> which inherits from this base configuration living in buildtools.
> This means that any small disagreements are not the end of the world,
> for example we could easily turn a rule on/off in adblockplusui if needs be.
> This is just an attempt at something sensible for us to start from.

While we probably need some project specific configuration to exclude
third-party and generated code specific to each project, I think we should agree
on a set of rules that we then use consistently across our projects. After all,
our coding practices are global, and not per project either, and ESLint is just
a tool to enforce them easier and more reliable. There is an exception for
website code though, at least for the time being, as we have to support legacy
browsers there, while we insist on some modern JavaScript features in our
extension code.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:16: "computed-property-spacing": "error",
On 2017/02/03 10:24:13, Wladimir Palant wrote:
> It's a pity that the consensus was apparently to remove consistent-return, I
> consider that rule very useful and an occasional explicit "return undefined"
not
> a bad thing.

I generally agree that it makes the code flow more clear if a function that is
called normally, always returns explicitly, if there is a potential return
value. But Dave and I agree, that this doesn't hold true for hooks that are
designed for optional return values (webRequest API, Promise.then(), ours
messaging.Port, etc.). in that case, we feel that the redundant "return
undefined" makes our code more clumsy. While we tested the previously suggested
configuration (with consistent-return enabled) on our code base, we didn't find
any instance of the former scenario, but quite some instances where we agreed
that it would make our code less preferable.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/03 10:24:13, Wladimir Palant wrote:
> Is this a good idea? Lexical declarations inside case clauses are disallowed
for
> a reason - a case clause doesn't have its own lexical scope, only the switch
> statement does. So the following is an error because it redeclares variable x:
> 
> switch (foo)
> {
>   case 1:
>     let x = 1;
>     break;
>   case 2:
>     let x = 2;
>     break;
> }
> 
> The correct way of writing this requires an additional block:
> 
> 
> switch (foo)
> {
>   case 1:
>   {
>     let x = 1;
>     break;
>   }
>   case 2:
>   {
>     let x = 2;
>     break;
>   }
> }

There is no difference (or potential footgun) in this example, as the code
"breaks" before another "case" would be reached. Hence there is no way another
"case" is executed after the variable declaration, that would cause the variable
to be defined or not dependent on how that code is reached. Still ESLint would
complain, with no-case-declarations turned on. There is only a potential
footgun, if you have any code in a "case" that does not (or might not) break,
which is already prohibited by another rule. Also see my related comment on that
rule, in the earlier discussion, on the other review.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:31: "no-cond-assign": "off",
On 2017/02/03 10:24:13, Wladimir Palant wrote:
> Frankly, I'd rather not disable this. An assignment inside a condition is
rarely
> intended, and even when it is it's bad for readability.

From the earlier discussion: While I might agree that assigning in an
if-statement sometimes results in somewhat cryptic code, I think for while-loops
following pattern is totally fine:

  let match;
  while (match = regex.exec(message))
  {
    doSomthing(match[1]);
  }

In fact, I think it is preferable over the alternative:

  while (true)
  {
    let match = regex.exec(message);
    if (!match)
      break;
    doSomething(match[1]);
  }

This is not only more verbose, but generally it is less obvious when/if the loop
terminates, if the exit condition isn't in the head.

Also note while testing this rule on our code base, we didn't find a single
inappropriate use of assignment in a condition statement, but just the pattern
above. So we agreed, that this rule would rather make our code worse.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/03 10:24:13, Wladimir Palant wrote:
> Why is this rule turned off? Flagging undeclared variables is the most
important
> feature of a linter. Any variables coming from external scopes should either
be
> covered by the environment or /* global */ comments.

While we tested that rule on our code base, we noticed a huge amount of errors
indicating use of undefined variables. These include global names defined in
other scripts, names injected when bundling modules during the build process,
and built-in browser APIs. The required boilerplate to get rid of those errors
would be significant and repetitive, Dave had a go.

However, as long as the code path gets executed during testing, there is no way
that you use an undefined name unnoticed, as opposed to the more sneaky quality
issues catched by ESLint. So I don't think it's worth to add the necessary
boilerplate to make ESLint happy with no-undef turned on. This was the consensus
of the previous discussion.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:71: "prefer-spread": "error",
On 2017/02/03 10:24:13, Wladimir Palant wrote:
> What about prefer-rest-params?

Well spotted. We had it in there before, and I don't recall that we discussed
removing it, neither do I see why we should.

[Wladimir Palant, 2017-02-03 13:01:34]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/03 12:49:32, Sebastian Noack wrote:
> There is no difference (or potential footgun) in this example, as the code
> "breaks" before another "case" would be reached. Hence there is no way another
> "case" is executed after the variable declaration, that would cause the
variable
> to be defined or not dependent on how that code is reached.

Try it ;)

This code will actually error out at compile time because of "redeclaration of
let x." While technically there might be no issue, declaring the same let
variable multiple times within the same lexical scope isn't allowed.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/03 12:49:31, Sebastian Noack wrote:
> While we tested that rule on our code base, we noticed a huge amount of errors
indicating use of undefined variables.

This indicates an issue. Built-in browser APIs and CommonJS should be covered by
a proper environment setting. If then we still have a huge amount of errors then
something is wrong with our code, different parts should normally be fairly
self-contained.

> So I don't think it's worth to add the necessary
> boilerplate to make ESLint happy with no-undef turned on. This was the
consensus
> of the previous discussion.

That boilerplate actually has some benefit: it shows human readers what external
dependencies there are.

[kzar, 2017-02-04 03:18:55]

Patch Set 2 : Addressed Wladimir's feedback

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:16: "computed-property-spacing": "error",
On 2017/02/03 12:49:32, Sebastian Noack wrote:
> On 2017/02/03 10:24:13, Wladimir Palant wrote:
> > It's a pity that the consensus was apparently to remove consistent-return, I
> > consider that rule very useful and an occasional explicit "return undefined"
> not
> > a bad thing.
> 
> I generally agree that it makes the code flow more clear if a function that is
> called normally, always returns explicitly, if there is a potential return
> value. But Dave and I agree, that this doesn't hold true for hooks that are
> designed for optional return values (webRequest API, Promise.then(), ours
> messaging.Port, etc.). in that case, we feel that the redundant "return
> undefined" makes our code more clumsy. While we tested the previously
suggested
> configuration (with consistent-return enabled) on our code base, we didn't
find
> any instance of the former scenario, but quite some instances where we agreed
> that it would make our code less preferable.

I don't want to use the consistent-return rule for adblockpluschrome, like
Sebastian mentioned it's a pain there. (If you insist we can enable it generally
and then turn it off again in the adblockpluschrome configuration though.)

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/03 13:01:34, Wladimir Palant wrote:
> On 2017/02/03 12:49:32, Sebastian Noack wrote:
> > There is no difference (or potential footgun) in this example, as the code
> > "breaks" before another "case" would be reached. Hence there is no way
another
> > "case" is executed after the variable declaration, that would cause the
> variable
> > to be defined or not dependent on how that code is reached.
> 
> Try it ;)
> 
> This code will actually error out at compile time because of "redeclaration of
> let x." While technically there might be no issue, declaring the same let
> variable multiple times within the same lexical scope isn't allowed.

I don't mind too much either way about this rule, so I've turned it back on for
now. Done.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:31: "no-cond-assign": "off",
On 2017/02/03 12:49:32, Sebastian Noack wrote:
> On 2017/02/03 10:24:13, Wladimir Palant wrote:
> > Frankly, I'd rather not disable this. An assignment inside a condition is
> rarely
> > intended, and even when it is it's bad for readability.
> 
> From the earlier discussion: While I might agree that assigning in an
> if-statement sometimes results in somewhat cryptic code, I think for
while-loops
> following pattern is totally fine:
> 
>   let match;
>   while (match = regex.exec(message))
>   {
>     doSomthing(match[1]);
>   }
> 
> In fact, I think it is preferable over the alternative:
> 
>   while (true)
>   {
>     let match = regex.exec(message);
>     if (!match)
>       break;
>     doSomething(match[1]);
>   }
> 
> This is not only more verbose, but generally it is less obvious when/if the
loop
> terminates, if the exit condition isn't in the head.
> 
> Also note while testing this rule on our code base, we didn't find a single
> inappropriate use of assignment in a condition statement, but just the pattern
> above. So we agreed, that this rule would rather make our code worse.

I agree with Sebastian here, it's sometimes a useful pattern. I'd prefer to
leave this rule off as well.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/03 13:01:34, Wladimir Palant wrote:
> On 2017/02/03 12:49:31, Sebastian Noack wrote:
> > While we tested that rule on our code base, we noticed a huge amount of
errors
> indicating use of undefined variables.
> 
> This indicates an issue. Built-in browser APIs and CommonJS should be covered
by
> a proper environment setting. If then we still have a huge amount of errors
then
> something is wrong with our code, different parts should normally be fairly
> self-contained.
> > So I don't think it's worth to add the necessary
> > boilerplate to make ESLint happy with no-undef turned on. This was the
> consensus
> > of the previous discussion.
> 
> That boilerplate actually has some benefit: it shows human readers what
external
> dependencies there are.

I can see arguments either way for this rule and while I'd rather leave it off I
won't insist. So in the interest of progress I've turned it back on for now.
Done.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:71: "prefer-spread": "error",
On 2017/02/03 12:49:32, Sebastian Noack wrote:
> On 2017/02/03 10:24:13, Wladimir Palant wrote:
> > What about prefer-rest-params?
> 
> Well spotted. We had it in there before, and I don't recall that we discussed
> removing it, neither do I see why we should.

I omitted this rule since I assumed rest parameters wouldn't work for code like
the WebSocket wrapper in adblockpluschrome/include.preload.js. But I've just
checked and I think I was wrong since `(function(a, ...b) { }).length == 1`.
Done.

[Wladimir Palant, 2017-02-04 07:38:19]

LGTM

[Sebastian Noack, 2017-02-04 16:43:38]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/04 03:18:55, kzar wrote:
> On 2017/02/03 13:01:34, Wladimir Palant wrote:
> > On 2017/02/03 12:49:32, Sebastian Noack wrote:
> > > There is no difference (or potential footgun) in this example, as the code
> > > "breaks" before another "case" would be reached. Hence there is no way
> another
> > > "case" is executed after the variable declaration, that would cause the
> > variable
> > > to be defined or not dependent on how that code is reached.
> > 
> > Try it ;)
> > 
> > This code will actually error out at compile time because of "redeclaration
of
> > let x." While technically there might be no issue, declaring the same let
> > variable multiple times within the same lexical scope isn't allowed.
> 
> I don't mind too much either way about this rule, so I've turned it back on
for
> now. Done.

Wladimir is right that in his example omitting the braces will actually produce
a syntax error. But you don't need a linter to find syntax errors, and in
particular I'm not sure whether this justifies to add a redundant block to each
"case" in which a new variable is declared.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/04 03:18:55, kzar wrote:
> On 2017/02/03 13:01:34, Wladimir Palant wrote:
> > On 2017/02/03 12:49:31, Sebastian Noack wrote:
> > > While we tested that rule on our code base, we noticed a huge amount of
> errors
> > indicating use of undefined variables.
> > 
> > This indicates an issue. Built-in browser APIs and CommonJS should be
covered
> by
> > a proper environment setting. If then we still have a huge amount of errors
> then
> > something is wrong with our code, different parts should normally be fairly
> > self-contained.
> > > So I don't think it's worth to add the necessary
> > > boilerplate to make ESLint happy with no-undef turned on. This was the
> > consensus
> > > of the previous discussion.
> > 
> > That boilerplate actually has some benefit: it shows human readers what
> external
> > dependencies there are.
> 
> I can see arguments either way for this rule and while I'd rather leave it off
I
> won't insist. So in the interest of progress I've turned it back on for now.
> Done.

IMO, the job of a linter is to ensure good coding practices, not necessarily to
find bugs. So I lend against any rule, that tends to generate false positives,
or requires additional boilerplate to avoid those, in particular if the issue
the rule aims to detect will come up during testing anyway.

But for the matter of progress, lets revisit this later. I would like to see the
complete changes to adblockpluschrome in order to make ESLint pass with this
configuration anyway, before approving it. So let's first see how bad the
related boilerplate turns out.

However, if there is any rule we cannot agree on (and that holds true as well
for the other controversial rule above, and any rule we are still going to
discuss), we should not use it. Let's try to enforce the rules we agree on,
rather than trying to make ESLint as strict as possible, in an infinite
bike-shedding discussion, resulting in unpopular compromises. After all this
will define the baseline for our coding practices that everybody has to accept.

[Wladimir Palant, 2017-02-04 21:13:13]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/04 16:43:37, Sebastian Noack wrote:
> Wladimir is right that in his example omitting the braces will actually
produce
> a syntax error. But you don't need a linter to find syntax errors, and in
> particular I'm not sure whether this justifies to add a redundant block to
each
> "case" in which a new variable is declared.

The point of a linter is catching errors before you run the code. Besides, this
syntax error isn't the biggest issue - consider a variable declared in one case
statement and used in another. This is a valid construct, yet chances are that
somebody simply forgot to declare this variable in the second case statement.
This kind of interaction between different case statements is a highly
unexpected language "feature" which is why this rule and the "redundant" block
make lots of sense IMHO.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/04 16:43:37, Sebastian Noack wrote:
> IMO, the job of a linter is to ensure good coding practices, not necessarily
to
> find bugs.

I see, that's the main reason for our disagreements it seems. Thing is,
JavaScript is a highly dynamic language which makes it hard to catch mistakes at
"compile time." IMHO, one large advantage of linting JavaScript is that you can
catch some coding mistakes before you even run the code. And the assumption that
each code path will be executed in testing is regularly being proven wrong.

[Sebastian Noack, 2017-02-05 18:22:39]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/04 21:13:13, Wladimir Palant wrote:
> On 2017/02/04 16:43:37, Sebastian Noack wrote:
> > Wladimir is right that in his example omitting the braces will actually
> produce
> > a syntax error. But you don't need a linter to find syntax errors, and in
> > particular I'm not sure whether this justifies to add a redundant block to
> each
> > "case" in which a new variable is declared.
> 
> The point of a linter is catching errors before you run the code. Besides,
this
> syntax error isn't the biggest issue - consider a variable declared in one
case
> statement and used in another. This is a valid construct, yet chances are that
> somebody simply forgot to declare this variable in the second case statement.
> This kind of interaction between different case statements is a highly
> unexpected language "feature" which is why this rule and the "redundant" block
> make lots of sense IMHO.

I completely agree, having code that might fall through after a case statement,
is a footgun, not only when there is a variable declaration. However, as I told
you before, this is already catched by another rule, i.e. no-falltrough. Also I
just noticed that the redeclaration in your example will be detected by
no-redeclare. Both rules are turned on, as they are part of eslint:recommended.
So in fact, there aren't any issues, we need no-case-declaration for, to catch.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/04 21:13:13, Wladimir Palant wrote:
> On 2017/02/04 16:43:37, Sebastian Noack wrote:
> > IMO, the job of a linter is to ensure good coding practices, not necessarily
> to
> > find bugs.
> 
> I see, that's the main reason for our disagreements it seems. Thing is,
> JavaScript is a highly dynamic language which makes it hard to catch mistakes
at
> "compile time." IMHO, one large advantage of linting JavaScript is that you
can
> catch some coding mistakes before you even run the code. And the assumption
that
> each code path will be executed in testing is regularly being proven wrong.

If we can catch some errors early, that is a nice side-effect of linting of
course. But I'd rather leverage the dynamic of JavaScript in order to keep the
code simple, concise and avoid repetition rather than limiting those benefits
through linting.

After all, accidental usage of undefined variables isn't the only cause of
potential errors. So there is no way around testing every code path anyway, in
order to make sure that it works, also if this might not always have happened.

Anyway, as I said before, I'm happy to defer this discussion, until we see the
exact boilerplate we'd have to add in order to use no-undef.

[kzar, 2017-02-06 06:21:01]

Patch Set 3 : Added browser env

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/05 18:22:39, Sebastian Noack wrote:
> Anyway, as I said before, I'm happy to defer this discussion, 
> until we see the exact boilerplate we'd have to add in order to 
> use no-undef.

I've added the boilerplate required for no-undef in my branch now,
https://github.com/kzar/adblockpluschrome/tree/4864-eslint . (Not linking the
commit since I'm force-pushing to that quite often at the moment.)

The changes weren't as bad as I originally thought, I wouldn't mind leaving
no-undef on.

[Wladimir Palant, 2017-02-06 09:55:20]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/05 18:22:39, Sebastian Noack wrote:
> I completely agree, having code that might fall through after a case
statement,
> is a footgun,

That's not what I meant. Here is the actual code:

switch (foo)
{
  case 1:
    let x = 2;
    break;
  case 2:
    x += 4;
    break;
}

Feel free to test whether any other rules catch this, normally all you get is a
runtime error "ReferenceError: can't access lexical declaration `x' before
initialization" (only if that code path is actually executed).

https://codereview.adblockplus.org/29374555/diff/29374619/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374619/.eslintrc.json#newc...
.eslintrc.json:5: "browser": true
This is wrong IMHO - this environment should be specified in the respective
projects. E.g. in Firefox we are currently running in a pure CommonJS
environment, no browser APIs available by default.

[kzar, 2017-02-06 10:00:27]

Patch Set 4 : Removed browser env again

https://codereview.adblockplus.org/29374555/diff/29374619/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374619/.eslintrc.json#newc...
.eslintrc.json:5: "browser": true
On 2017/02/06 09:55:19, Wladimir Palant wrote:
> This is wrong IMHO - this environment should be specified in the respective
> projects. E.g. in Firefox we are currently running in a pure CommonJS
> environment, no browser APIs available by default.

Done.

[Sebastian Noack, 2017-02-06 13:54:43]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/06 09:55:19, Wladimir Palant wrote:
> On 2017/02/05 18:22:39, Sebastian Noack wrote:
> > I completely agree, having code that might fall through after a case
> statement,
> > is a footgun,
> 
> That's not what I meant. Here is the actual code:
> 
> switch (foo)
> {
>   case 1:
>     let x = 2;
>     break;
>   case 2:
>     x += 4;
>     break;
> }
> 
> Feel free to test whether any other rules catch this, normally all you get is
a
> runtime error "ReferenceError: can't access lexical declaration `x' before
> initialization" (only if that code path is actually executed).

Regardless whether you put a redundant block around the first case or not (and
that is the only thing no-case-declarations would complain about here), you
still get the same ReferenceError at runtime. Your example hasn't much to do
with lexical declarations in case statements, but rather with usage of undefined
variables. If we decide to go ahead with no-undef, it would detect the issue
here, but that is another discussion. Either way no-case-declarations isn't any
useful here. So can we please finally agree to turn it off again?

[kzar, 2017-02-06 16:26:13]

Patch Set 5 : Switch no-case-declarations off

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/06 13:54:43, Sebastian Noack wrote:
> If we decide to go ahead with no-undef, it would detect the issue
> here, but that is another discussion. Either way no-case-
> declarations isn't any useful here. So can we please finally agree 
> to turn it off again?

Well honestly I don't mind too much either way. How about we turn
no-case-declarations off but leave no-undef on?

[Sebastian Noack, 2017-02-06 17:26:00]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:29: "no-case-declarations": "off",
On 2017/02/06 16:26:13, kzar wrote:
> On 2017/02/06 13:54:43, Sebastian Noack wrote:
> > If we decide to go ahead with no-undef, it would detect the issue
> > here, but that is another discussion. Either way no-case-
> > declarations isn't any useful here. So can we please finally agree 
> > to turn it off again?
> 
> Well honestly I don't mind too much either way. How about we turn
> no-case-declarations off but leave no-undef on?

Honestly, I cannot see how this is still a controversial decision. After
clearing up some wrong assumptions, we established that there is not a single
potential issue caught by no-case-declarations (that isn't redundantly
identified by other rules). At the same time the additional boilerplate enforced
by no-case-declarations is redundant and potentially inconsistent. Turning this
rule off, at the current point, should be a no-brainer. However, no-undef is a
whole different story and perhaps a more controversial rule.

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/06 06:21:01, kzar wrote:
> On 2017/02/05 18:22:39, Sebastian Noack wrote:
> > Anyway, as I said before, I'm happy to defer this discussion, 
> > until we see the exact boilerplate we'd have to add in order to 
> > use no-undef.
> 
> I've added the boilerplate required for no-undef in my branch now,
> https://github.com/kzar/adblockpluschrome/tree/4864-eslint . (Not linking the
> commit since I'm force-pushing to that quite often at the moment.)
> 
> The changes weren't as bad as I originally thought, I wouldn't mind leaving
> no-undef on.

Thanks. Indeed, the configuration for environment-specific globals is quite
straight-forward. It would be even better, though, if you'd add the "commonjs"
(includes "exports" and "requires") and "webextensions" (includes "chrome" and
"browser") environments, rather than explicitly listing the respective globals.

But the boilerplate to configure our own globals from other scripts is still too
much. This effects mostly places (like content scripts and the user interface)
where we don't organize the code in modules, which isn't great in the first
place. So how about bundling that code as modules as well? This will make the
code better organized, and eliminate the need for most globals, in one go.

[kzar, 2017-02-07 06:37:59]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/06 17:26:00, Sebastian Noack wrote:
> Thanks. Indeed, the configuration for environment-specific globals is quite
> straight-forward. It would be even better, though, if you'd add the "commonjs"
> (includes "exports" and "requires") and "webextensions" (includes "chrome" and
> "browser") environments, rather than explicitly listing the respective
globals.
> 
> But the boilerplate to configure our own globals from other scripts is still
too
> much. This effects mostly places (like content scripts and the user interface)
> where we don't organize the code in modules, which isn't great in the first
> place. So how about bundling that code as modules as well? This will make the
> code better organized, and eliminate the need for most globals, in one go.

Sure, we can improve on the work in adblockpluschrome (and I'm about to have a
look at your feedback on GitHub for that) but it's really a separate issue
(#4864). For now I'm just hoping we can find a common set of rules for this base
ESLint configuration so that we can get it pushed.

- It seems we agree that the burden of no-undef compliance isn't too large so
how about we leave it enabled?
- It seems no-case-declarations is contested so let's just leave it disabled.

Is that a suitable compromise that you'd both be happy for this to land? If so
I'd appreciate a looks good, if not please suggest a compromise that might work
for you both.

[Sebastian Noack, 2017-02-07 11:57:15]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/07 06:37:58, kzar wrote:
> On 2017/02/06 17:26:00, Sebastian Noack wrote:
> > Thanks. Indeed, the configuration for environment-specific globals is quite
> > straight-forward. It would be even better, though, if you'd add the
"commonjs"
> > (includes "exports" and "requires") and "webextensions" (includes "chrome"
and
> > "browser") environments, rather than explicitly listing the respective
> globals.
> > 
> > But the boilerplate to configure our own globals from other scripts is still
> too
> > much. This effects mostly places (like content scripts and the user
interface)
> > where we don't organize the code in modules, which isn't great in the first
> > place. So how about bundling that code as modules as well? This will make
the
> > code better organized, and eliminate the need for most globals, in one go.
> 
> Sure, we can improve on the work in adblockpluschrome (and I'm about to have a
> look at your feedback on GitHub for that) but it's really a separate issue
> (#4864). For now I'm just hoping we can find a common set of rules for this
base
> ESLint configuration so that we can get it pushed.
> 
> - It seems we agree that the burden of no-undef compliance isn't too large so
> how about we leave it enabled?
> - It seems no-case-declarations is contested so let's just leave it disabled.
> 
> Is that a suitable compromise that you'd both be happy for this to land? If so
> I'd appreciate a looks good, if not please suggest a compromise that might
work
> for you both.

The linter configuration defines our coding practices, and in order to evaluate
the suggested practices/configuration, I need to review the changes it implies
on our code, first. If you could put up a combined patch, with the changes for
adblokpluschrome, for review, that would be helpful. Doing codereview on GitHub
is quite inconvenient for me, also I'd prefer to review all changes together. As
for no-undef, I'm fine with it, under the condition that we significantly reduce
the usage of global variables in our code (e.g. through modules).

[kzar, 2017-02-07 17:07:52]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/07 11:57:15, Sebastian Noack wrote:
> On 2017/02/07 06:37:58, kzar wrote:
> > On 2017/02/06 17:26:00, Sebastian Noack wrote:
> > > Thanks. Indeed, the configuration for environment-specific globals is
quite
> > > straight-forward. It would be even better, though, if you'd add the
> "commonjs"
> > > (includes "exports" and "requires") and "webextensions" (includes "chrome"
> and
> > > "browser") environments, rather than explicitly listing the respective
> > globals.
> > > 
> > > But the boilerplate to configure our own globals from other scripts is
still
> > too
> > > much. This effects mostly places (like content scripts and the user
> interface)
> > > where we don't organize the code in modules, which isn't great in the
first
> > > place. So how about bundling that code as modules as well? This will make
> the
> > > code better organized, and eliminate the need for most globals, in one go.
> > 
> > Sure, we can improve on the work in adblockpluschrome (and I'm about to have
a
> > look at your feedback on GitHub for that) but it's really a separate issue
> > (#4864). For now I'm just hoping we can find a common set of rules for this
> base
> > ESLint configuration so that we can get it pushed.
> > 
> > - It seems we agree that the burden of no-undef compliance isn't too large
so
> > how about we leave it enabled?
> > - It seems no-case-declarations is contested so let's just leave it
disabled.
> > 
> > Is that a suitable compromise that you'd both be happy for this to land? If
so
> > I'd appreciate a looks good, if not please suggest a compromise that might
> work
> > for you both.
> 
> The linter configuration defines our coding practices, and in order to
evaluate
> the suggested practices/configuration, I need to review the changes it implies
> on our code, first. If you could put up a combined patch, with the changes for
> adblokpluschrome, for review, that would be helpful. Doing codereview on
GitHub
> is quite inconvenient for me, also I'd prefer to review all changes together.
As
> for no-undef, I'm fine with it, under the condition that we significantly
reduce
> the usage of global variables in our code (e.g. through modules).

It's a WIP but here you go https://codereview.adblockplus.org/29374674/

[kzar, 2017-02-09 04:55:28]

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374555/diff/29374556/.eslintrc.json#newc...
.eslintrc.json:49: "no-undef": "off",
On 2017/02/07 17:07:51, kzar wrote:
> On 2017/02/07 11:57:15, Sebastian Noack wrote:
> > The linter configuration defines our coding practices, and in
> > order to evaluate the suggested practices/configuration, I need to
> > review the changes it implies on our code, first. If you could put
> > up a combined patch, with the changes for adblokpluschrome, for
> > review, that would be helpful. Doing codereview on GitHub is quite
> > inconvenient for me, also I'd prefer to review all changes
> > together.
>
> It's a WIP but here you go https://codereview.adblockplus.org/29374674/

Since you've now gone through and reviewed the adblockpluschrome changes would
you mind giving this base ESLint configuration a looks good? While I'm still in
the process of addressing your feedback I can't see anything there that should
block this review, and in fact those changes can't land before this does anyway.

[Sebastian Noack, 2017-02-09 11:30:56]

I wonder where we should put the ESLint configuration. It seems the initial idea
was to have it in codingtools, but the new patch suggests to land it in
buildtools.

As far as our browser extensions are concerned, having it in buildtools comes
handy, as buildtools is pulled in as dependency anyway in adblockpluschrome,
adblockplus, adblockpluscore and adblockplusui.

However, I hope it's still the the plan to use the ESLint configuration for our
websites as well (either a derived version targeting ES5, or even using the same
rules in combination with a transpiler). But then buildtools seems the wrong
place as it is specific to browser extensions, while coding tools already
provides other linting functionality (i.e. flake8-abp).

[kzar, 2017-02-09 12:09:35]

On 2017/02/09 11:30:56, Sebastian Noack wrote:
> I wonder where we should put the ESLint configuration. It seems the initial
idea
> was to have it in codingtools, but the new patch suggests to land it in
> buildtools.
> 
> As far as our browser extensions are concerned, having it in buildtools comes
> handy, as buildtools is pulled in as dependency anyway in adblockpluschrome,
> adblockplus, adblockpluscore and adblockplusui.
> 
> However, I hope it's still the the plan to use the ESLint configuration for
our
> websites as well (either a derived version targeting ES5, or even using the
same
> rules in combination with a transpiler). But then buildtools seems the wrong
> place as it is specific to browser extensions, while coding tools already
> provides other linting functionality (i.e. flake8-abp).

I plan to put this base ESLint configuration in buildtools. 

As mentioned in the codereview discussions and in the issue description this
configuration is only for projects using modern JavaScript, specifically the
adblockplus, adblockpluschrome, adblockpluscore and adblockplusui repositories.

- Like you mentioned all those repositories inherit from buildtools so it's a
convenient place to put it. (Having it there allows us to use ESLint
configuration inheritance.)
- I hope to have buildtools automatically lint the code at build time in the
future.

Perhaps one day we'll need to reconsider and move the configuration to
codingtools. If so we can then add codingtools as a dependency to buildtools. I
think it's premature to worry about that now however.

[Sebastian Noack, 2017-02-09 15:51:30]

From the discussion on IRC, it seems we prefer to have the configuration in
codingtools. This seems to be where it belongs, as buildtools is specific to the
Adblock Plus browser extensions, but ESLint and codingtools are not, even though
the current configuration is targeting only modern code. Not to mention other
linting already implemented in codingtools.

The only reason to put the configuration into buildtools would be easy
integration with adblockplus[chrome|core|ui] where buildtools is pulled in
anyway, so that we can just extend from it using a relative path. However,
creating an artificial dependency on buildtools, from Flattr+ or website
projects, just for the ESLint configuration, seems inappropriate. Pulling in the
whole codingtools repository (through ensure_dependcies.py), for this purpose,
isn't any better either.

It seems the recommended way to share ESLint configurations across projects is
to package them as node module [1]. This would also address the concerns above.
And you have to use npm anyway for eslint, even in projects that don't use node
otherwise. And in projects where npm is already used (i.e. Flattr+,
adblockpluscore and abp2blocklist) this seems to be the canonical way, in the
first place.

[1]: http://eslint.org/docs/developer-guide/shareable-configs

[kzar, 2017-02-10 14:54:03]

Patch Set 6 : Move configuration to a Node.js module in the codingtools
repository

[Sebastian Noack, 2017-02-10 18:04:32]

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/README.md:4: [Adblock Plus coding style
guide](https://adblockplus.org/coding-style#javascript).
This seems a little inconsistent with the decision to name it eslint-config-eyeo
(rather than eslint-config-abp). If we stick to that name, but don't move the
styleguide to eyeo.com yet (we won't), perhaps we should at least clarify that
it's the coding style used by all Eyeo projects, to avoid confusion.

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/README.md:21: Note: If you need to lint files inside projects
which do not already have an
This is not only relevant when linting projects that don't have an ESLint
configuration, but that is also the minimal boilerplate when creating an ESLint
configuration for a project. Mind clarifying that?

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/index.js:33: camelcase: ["error", {properties: "never"}],
It may be against the ESLint configuration we define here, but it certainly
looks weird to omit the quotes for the rules that don't have a dash in their
name, while putting the vast majority of rules in quotes here.

[kzar, 2017-02-10 19:10:45]

Patch Set 7 : Addressed Sebastian's feedback

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/README.md:4: [Adblock Plus coding style
guide](https://adblockplus.org/coding-style#javascript).
On 2017/02/10 18:04:31, Sebastian Noack wrote:
> This seems a little inconsistent with the decision to name it
eslint-config-eyeo
> (rather than eslint-config-abp). If we stick to that name, but don't move the
> styleguide to http://eyeo.com yet (we won't), perhaps we should at least
clarify that
> it's the coding style used by all Eyeo projects, to avoid confusion.

Done.

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/README.md:21: Note: If you need to lint files inside projects
which do not already have an
On 2017/02/10 18:04:31, Sebastian Noack wrote:
> This is not only relevant when linting projects that don't have an ESLint
> configuration, but that is also the minimal boilerplate when creating an
ESLint
> configuration for a project. Mind clarifying that?

Done.

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/index.js:33: camelcase: ["error", {properties: "never"}],
On 2017/02/10 18:04:31, Sebastian Noack wrote:
> It may be against the ESLint configuration we define here, but it certainly
> looks weird to omit the quotes for the rules that don't have a dash in their
> name, while putting the vast majority of rules in quotes here.

Yea I agree. I think this is kind of a special case so I've changed quote-props
to "always" for this file.

[Sebastian Noack, 2017-02-12 20:53:12]

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/index.js:33: camelcase: ["error", {properties: "never"}],
IOn 2017/02/10 19:10:45, kzar wrote:
> On 2017/02/10 18:04:31, Sebastian Noack wrote:
> > It may be against the ESLint configuration we define here, but it certainly
> > looks weird to omit the quotes for the rules that don't have a dash in their
> > name, while putting the vast majority of rules in quotes here.
> 
> Yea I agree. I think this is kind of a special case so I've changed
quote-props
> to "always" for this file.

It seems the quote-props rule has the consistent-as-needed option to target this
very case. So that if some properties need to be quoted, all are required to be
quoted for consistency. The only reservation I'd have is that with just
as-needed, I spotted cases where plain objects were misused, and a Map object
would be more appropriate. On the other hand, this file isn't an exception in
that sense, but we merely have a requirement by an external library (ESLint
requires it to be an object). Considering that (and that we won't catch all
inappropriate use of plain objects that way, and that we might have to deal with
external APIs more often), I think we should rather use consistent-as-needed.

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/README.md:31: In the mean time if you need to lint files
inside projects which do
I don't like the duplication here. How about:

  In order to use eslint-config-eyeo,
  add following option to your .eslintrc.json
  (or put it in your ~/.eslintrc.json in order
  to lint projects that don't have an own
  configuration file):

(Or something along these lines) followed by the snippet.

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/index.js:61: "no-labels": "error",
I posted this rule by accident among the other rules I suggested to add. You
have to label outer loops in order to break out of it from an inner loop. While
we currently have no need for this (in adblockpluschrome), I think this is a
legit use case. The alternative is much more complex. And in fact we currently
have a patch under review where we have to break out of an outer loop.

[kzar, 2017-02-13 06:02:58]

Patch Set 8 : Addressed Sebastian's further feedback

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374927/eslint-config-eyeo/...
eslint-config-eyeo/index.js:33: camelcase: ["error", {properties: "never"}],
On 2017/02/12 20:53:11, Sebastian Noack wrote:
> IOn 2017/02/10 19:10:45, kzar wrote:
> > On 2017/02/10 18:04:31, Sebastian Noack wrote:
> > > It may be against the ESLint configuration we define here, but it
certainly
> > > looks weird to omit the quotes for the rules that don't have a dash in
their
> > > name, while putting the vast majority of rules in quotes here.
> > 
> > Yea I agree. I think this is kind of a special case so I've changed
> quote-props
> > to "always" for this file.
> 
> It seems the quote-props rule has the consistent-as-needed option to target
this
> very case. So that if some properties need to be quoted, all are required to
be
> quoted for consistency. The only reservation I'd have is that with just
> as-needed, I spotted cases where plain objects were misused, and a Map object
> would be more appropriate. On the other hand, this file isn't an exception in
> that sense, but we merely have a requirement by an external library (ESLint
> requires it to be an object). Considering that (and that we won't catch all
> inappropriate use of plain objects that way, and that we might have to deal
with
> external APIs more often), I think we should rather use consistent-as-needed.

Done.

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/README.md:31: In the mean time if you need to lint files
inside projects which do
On 2017/02/12 20:53:11, Sebastian Noack wrote:
> I don't like the duplication here. How about:
> 
>   In order to use eslint-config-eyeo,
>   add following option to your .eslintrc.json
>   (or put it in your ~/.eslintrc.json in order
>   to lint projects that don't have an own
>   configuration file):
> 
> (Or something along these lines) followed by the snippet.

OK, I've had another go. What do you think?

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/index.js:61: "no-labels": "error",
On 2017/02/12 20:53:12, Sebastian Noack wrote:
> I posted this rule by accident among the other rules I suggested to add. You
> have to label outer loops in order to break out of it from an inner loop.
While
> we currently have no need for this (in adblockpluschrome), I think this is a
> legit use case. The alternative is much more complex. And in fact we currently
> have a patch under review where we have to break out of an outer loop.

Good point. How about we leave this rule enabled but with the allowLoop option?

[kzar, 2017-02-13 08:37:26]

Patch Set 9 : Removed no-shadow rule

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:68: "no-shadow": "error",
I think the no-shadow rule might be a mistake. It sounds sensible but in
practice (looking at adblockpluscore and adblockplusui) I've found it leads to
pointlessly having to either give variables new names or to adding scopes.

For example take _onDownloadSucess in adblockpluscore/lib/synchronizer.js[1].
The `match` variable is shadowed inside the for loop, but I don't think that's
confusing or unclear. IMO the code is worse with the second variable renamed to
something like `otherMatch` or with a block scope added around the original
definition and use of the variable.

Opinions?

[1] -
https://github.com/adblockplus/adblockpluscore/blob/master/lib/synchronizer.j...

[kzar, 2017-02-13 09:27:09]

Patch Set 10 : Allow empty catch blocks

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
eslint-config-eyeo/index.js:56: "no-empty": ["error", {allowEmptyCatch: true}],
(Without this ESLint complained about empty blocks after a try... catch...,
which is something we do in adblockpluscore.)

[Felix Dahlke, 2017-02-13 09:51:20]

Don't want to get involved too deeply here so I'm staying in CC, but there's one
thing I wanted to discuss about a recent change to adblockplus.org/coding-style.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
How I see it [0], this makes the following invalid:

if (foo)
{
   one;
   two;
}
else
   three;

I personally prefer this style, and we have some examples for it in the code
base. What's the reasoning for changing it? (I'm aware that this has been added
to adblockplus.org/coding-style recently, a change I did not review however).

I'm OK with changing it as long as there's a reason for it. Was there a
discussion about this you can point me to?

[0]: http://eslint.org/docs/rules/curly#consistent

[kzar, 2017-02-13 10:07:50]

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/13 09:51:20, Felix Dahlke wrote:
> How I see it [0], this makes the following invalid:
> 
> if (foo)
> {
>    one;
>    two;
> }
> else
>    three;
> 
> I personally prefer this style, and we have some examples for it in the code
> base. What's the reasoning for changing it? (I'm aware that this has been
added
> to adblockplus.org/coding-style recently, a change I did not review however).
> 
> I'm OK with changing it as long as there's a reason for it. Was there a
> discussion about this you can point me to?
> 
> [0]: http://eslint.org/docs/rules/curly#consistent

I don't have any links to hand but IIRC both Thomas and Wladimir told me to use
braces for if...else... clauses consistently in codereviews in the past. It's
true that our codebase varies quite a bit with brace usage, but I guess that's a
good example of why agreeing on the linting rules now will help us going
forwards.

Personally I prefer that braces to be consistently used, but I don't insist
either. What does everyone else think?

[Sebastian Noack, 2017-02-13 11:31:30]

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/index.js:61: "no-labels": "error",
On 2017/02/13 06:02:58, kzar wrote:
> On 2017/02/12 20:53:12, Sebastian Noack wrote:
> > I posted this rule by accident among the other rules I suggested to add. You
> > have to label outer loops in order to break out of it from an inner loop.
> While
> > we currently have no need for this (in adblockpluschrome), I think this is a
> > legit use case. The alternative is much more complex. And in fact we
currently
> > have a patch under review where we have to break out of an outer loop.
> 
> Good point. How about we leave this rule enabled but with the allowLoop
option?

Yeah, at very least we should add the "allowLoop" option. But we might just
remove this rule altogether, if it is just to avoid unnecessary complexity in
the ESLint configuration. Labeling random blocks isn't quite a common practice,
and inappropriate use of labels is unlikely to make it past our code review or
to create controversy during the review. No strong opinion though.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/13 10:07:50, kzar wrote:
> On 2017/02/13 09:51:20, Felix Dahlke wrote:
> > How I see it [0], this makes the following invalid:
> > 
> > if (foo)
> > {
> >    one;
> >    two;
> > }
> > else
> >    three;
> > 
> > I personally prefer this style, and we have some examples for it in the code
> > base. What's the reasoning for changing it? (I'm aware that this has been
> added
> > to adblockplus.org/coding-style recently, a change I did not review
however).
> > 
> > I'm OK with changing it as long as there's a reason for it. Was there a
> > discussion about this you can point me to?
> > 
> > [0]: http://eslint.org/docs/rules/curly#consistent
> 
> I don't have any links to hand but IIRC both Thomas and Wladimir told me to
use
> braces for if...else... clauses consistently in codereviews in the past. It's
> true that our codebase varies quite a bit with brace usage, but I guess that's
a
> good example of why agreeing on the linting rules now will help us going
> forwards.
> 
> Personally I prefer that braces to be consistently used, but I don't insist
> either. What does everyone else think?

Yeah, I remember as well, from past code reviews, that Wladimir and Thomas felt
strong about adding braces in this case. Personally, I don't care much. But if
we remove the "consistent" option, and the the related clause from our coding
style guide, I don't want to see people complaining about the lack of braces
there anymore. The whole point of this rule (and many others) is to streamline
our code review process.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:68: "no-shadow": "error",
On 2017/02/13 08:37:25, kzar wrote:
> I think the no-shadow rule might be a mistake. It sounds sensible but in
> practice (looking at adblockpluscore and adblockplusui) I've found it leads to
> pointlessly having to either give variables new names or to adding scopes.
> 
> For example take _onDownloadSucess in adblockpluscore/lib/synchronizer.js[1].
> The `match` variable is shadowed inside the for loop, but I don't think that's
> confusing or unclear. IMO the code is worse with the second variable renamed
to
> something like `otherMatch` or with a block scope added around the original
> definition and use of the variable.
> 
> Opinions?
> 
> [1] -
>
https://github.com/adblockplus/adblockpluscore/blob/master/lib/synchronizer.j...

In this particular case I think the code will be easier to understand when using
"headerMatch" and "commentMatch" respectively rather than reusing the same name.
If you still want to reuse the variable name, but I think that is a bad idea,
you can do so by just removing the let statement in the inner block, and ESLint
would no longer complain.

The problem with variable shadowing is that it often makes the code hard to get
right. You have to very carefully follow the variable declarations to know which
value a name will refer to. At times you will get bugs because the developer
assumes a variable to refer to a different value, not noticing that it has been
shadowed. There might be situations though, were it is hard to find a good name
for a variable, and the addional requirement for the name to be unique, might
encourage usage of bad names. So I might reconsider this rule, if you show me a
better example where this rule is problematic.

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
eslint-config-eyeo/README.md:27: "root": true
One could argue, documenting the root option here is out of scope. On the other
hand, pointing it out here might make it easier to get started with ESLint, but
then it seems incomplete, as you have to set the corresponding "env" as well.

[Felix Dahlke, 2017-02-13 12:05:16]

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/13 11:31:29, Sebastian Noack wrote:
> On 2017/02/13 10:07:50, kzar wrote:
> > On 2017/02/13 09:51:20, Felix Dahlke wrote:
> > > How I see it [0], this makes the following invalid:
> > > 
> > > if (foo)
> > > {
> > >    one;
> > >    two;
> > > }
> > > else
> > >    three;
> > > 
> > > I personally prefer this style, and we have some examples for it in the
code
> > > base. What's the reasoning for changing it? (I'm aware that this has been
> > added
> > > to adblockplus.org/coding-style recently, a change I did not review
> however).
> > > 
> > > I'm OK with changing it as long as there's a reason for it. Was there a
> > > discussion about this you can point me to?
> > > 
> > > [0]: http://eslint.org/docs/rules/curly#consistent
> > 
> > I don't have any links to hand but IIRC both Thomas and Wladimir told me to
> use
> > braces for if...else... clauses consistently in codereviews in the past.
It's
> > true that our codebase varies quite a bit with brace usage, but I guess
that's
> a
> > good example of why agreeing on the linting rules now will help us going
> > forwards.
> > 
> > Personally I prefer that braces to be consistently used, but I don't insist
> > either. What does everyone else think?
> 
> Yeah, I remember as well, from past code reviews, that Wladimir and Thomas
felt
> strong about adding braces in this case. Personally, I don't care much. But if
> we remove the "consistent" option, and the the related clause from our coding
> style guide, I don't want to see people complaining about the lack of braces
> there anymore. The whole point of this rule (and many others) is to streamline
> our code review process.

Yeah, that's the thing: If we deliberately decide to leave it up to the person
writing the code, we have to do just that. That's frankly how we've done it
until now, I'm not sure why Wladimir and Thomas apparently started to enforce
this.

With Wladimir I find this particularly hard to believe since he appears to
prefer the style without braces, I've seen that in code from him a few times.
For a recent example, see loadModules here:
https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js

To not blow the discussion up here, I'll ask them on IRC and report back here.

[kzar, 2017-02-13 13:02:22]

Patch Set 11 : Turn no-shadow back on

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/index.js:61: "no-labels": "error",
On 2017/02/13 11:31:29, Sebastian Noack wrote:
> On 2017/02/13 06:02:58, kzar wrote:
> > On 2017/02/12 20:53:12, Sebastian Noack wrote:
> > > I posted this rule by accident among the other rules I suggested to add.
You
> > > have to label outer loops in order to break out of it from an inner loop.
> > While
> > > we currently have no need for this (in adblockpluschrome), I think this is
a
> > > legit use case. The alternative is much more complex. And in fact we
> currently
> > > have a patch under review where we have to break out of an outer loop.
> > 
> > Good point. How about we leave this rule enabled but with the allowLoop
> option?
> 
> Yeah, at very least we should add the "allowLoop" option. But we might just
> remove this rule altogether, if it is just to avoid unnecessary complexity in
> the ESLint configuration. Labeling random blocks isn't quite a common
practice,
> and inappropriate use of labels is unlikely to make it past our code review or
> to create controversy during the review. No strong opinion though.

I've never seen a use-case for labels which didn't involve breaking out of
nested loops, so I think it's OK to leave the rule on with that option.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/13 12:05:16, Felix Dahlke wrote:
> With Wladimir I find this particularly hard to believe...
> 
> To not blow the discussion up here, I'll ask them on IRC and report 
> back here.

Yea, good idea. I'm not trying to put words in anyone's mouth and perhaps I'm
misremembering.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:68: "no-shadow": "error",
On 2017/02/13 11:31:29, Sebastian Noack wrote:
> On 2017/02/13 08:37:25, kzar wrote:
> > I think the no-shadow rule might be a mistake. It sounds sensible but in
> > practice (looking at adblockpluscore and adblockplusui) I've found it leads
to
> > pointlessly having to either give variables new names or to adding scopes.
> > 
> > For example take _onDownloadSucess in
adblockpluscore/lib/synchronizer.js[1].
> > The `match` variable is shadowed inside the for loop, but I don't think
that's
> > confusing or unclear. IMO the code is worse with the second variable renamed
> to
> > something like `otherMatch` or with a block scope added around the original
> > definition and use of the variable.
> > 
> > Opinions?
> > 
> > [1] -
> >
>
https://github.com/adblockplus/adblockpluscore/blob/master/lib/synchronizer.j...
> 
> In this particular case I think the code will be easier to understand when
using
> "headerMatch" and "commentMatch" respectively rather than reusing the same
name.
> If you still want to reuse the variable name, but I think that is a bad idea,
> you can do so by just removing the let statement in the inner block, and
ESLint
> would no longer complain.
> 
> The problem with variable shadowing is that it often makes the code hard to
get
> right. You have to very carefully follow the variable declarations to know
which
> value a name will refer to. At times you will get bugs because the developer
> assumes a variable to refer to a different value, not noticing that it has
been
> shadowed. There might be situations though, were it is hard to find a good
name
> for a variable, and the addional requirement for the name to be unique, might
> encourage usage of bad names. So I might reconsider this rule, if you show me
a
> better example where this rule is problematic.

Yea, you're probably right.

While the rule can be annoying it certainly can stop some tricky bugs too. I
can't see much better an example than I gave already. The rule is kind of
annoying to follow in places like removeFilter[1] in
adblockpluscore/lib/filterStorage.js but I don't think that justifies leaving
the rule off.

[1] -
https://github.com/adblockplus/adblockpluscore/blob/master/lib/filterStorage....

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375559/eslint-config-eyeo/...
eslint-config-eyeo/README.md:27: "root": true
On 2017/02/13 11:31:29, Sebastian Noack wrote:
> One could argue, documenting the root option here is out of scope. On the
other
> hand, pointing it out here might make it easier to get started with ESLint,
but
> then it seems incomplete, as you have to set the corresponding "env" as well.

Well most projects will need more in their configuration but this is a start.
That's why I described it as a minimal example. I guess we could start talking
about env and globals here too but IMO it's out of scope.

[Thomas Greiner, 2017-02-13 13:06:14]

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
> I don't have any links to hand but IIRC both Thomas and Wladimir told me to
use braces for if...else... clauses consistently in codereviews in the past.

I can only speak for myself but my objection was only against multi-line blocks
without braces to avoid potential issues when touching them in the future.

e.g.
for (let a of b)
  if (a == c)
    c = 1;
  else
    c = 2;

No opinion on single-line blocks without braces though.

[kzar, 2017-02-13 13:13:15]

On 2017/02/13 13:06:14, Thomas Greiner wrote:
> I can only speak for myself but my objection was only against multi-line
blocks
> without braces to avoid potential issues when touching them in the future.
> 
> e.g.
> for (let a of b)
>   if (a == c)
>     c = 1;
>   else
>     c = 2;
> 
> No opinion on single-line blocks without braces though.

But any opinion on inconsistent braces in an if...else... like this?

if (true)
  console.log(1);
else
{
  let a = 1;
  console.log(a);
}

[Sebastian Noack, 2017-02-13 14:20:29]

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374931/eslint-config-eyeo/...
eslint-config-eyeo/index.js:61: "no-labels": "error",
On 2017/02/13 13:02:21, kzar wrote:
> On 2017/02/13 11:31:29, Sebastian Noack wrote:
> > On 2017/02/13 06:02:58, kzar wrote:
> > > On 2017/02/12 20:53:12, Sebastian Noack wrote:
> > > > I posted this rule by accident among the other rules I suggested to add.
> You
> > > > have to label outer loops in order to break out of it from an inner
loop.
> > > While
> > > > we currently have no need for this (in adblockpluschrome), I think this
is
> a
> > > > legit use case. The alternative is much more complex. And in fact we
> > currently
> > > > have a patch under review where we have to break out of an outer loop.
> > > 
> > > Good point. How about we leave this rule enabled but with the allowLoop
> > option?
> > 
> > Yeah, at very least we should add the "allowLoop" option. But we might just
> > remove this rule altogether, if it is just to avoid unnecessary complexity
in
> > the ESLint configuration. Labeling random blocks isn't quite a common
> practice,
> > and inappropriate use of labels is unlikely to make it past our code review
or
> > to create controversy during the review. No strong opinion though.
> 
> I've never seen a use-case for labels which didn't involve breaking out of
> nested loops, so I think it's OK to leave the rule on with that option.

The same way, you could argue, because it is so rare, it's not worth even having
that rule. Or you could even go one step further, and argue because there are no
representative examples of either good or bad usage, protecting against labels
with a rule would be premature. ;) But I suppose it doesn't matter much.

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:68: "no-shadow": "error",
On 2017/02/13 13:02:22, kzar wrote:
> On 2017/02/13 11:31:29, Sebastian Noack wrote:
> > On 2017/02/13 08:37:25, kzar wrote:
> > > I think the no-shadow rule might be a mistake. It sounds sensible but in
> > > practice (looking at adblockpluscore and adblockplusui) I've found it
leads
> to
> > > pointlessly having to either give variables new names or to adding scopes.
> > > 
> > > For example take _onDownloadSucess in
> adblockpluscore/lib/synchronizer.js[1].
> > > The `match` variable is shadowed inside the for loop, but I don't think
> that's
> > > confusing or unclear. IMO the code is worse with the second variable
renamed
> > to
> > > something like `otherMatch` or with a block scope added around the
original
> > > definition and use of the variable.
> > > 
> > > Opinions?
> > > 
> > > [1] -
> > >
> >
>
https://github.com/adblockplus/adblockpluscore/blob/master/lib/synchronizer.j...
> > 
> > In this particular case I think the code will be easier to understand when
> using
> > "headerMatch" and "commentMatch" respectively rather than reusing the same
> name.
> > If you still want to reuse the variable name, but I think that is a bad
idea,
> > you can do so by just removing the let statement in the inner block, and
> ESLint
> > would no longer complain.
> > 
> > The problem with variable shadowing is that it often makes the code hard to
> get
> > right. You have to very carefully follow the variable declarations to know
> which
> > value a name will refer to. At times you will get bugs because the developer
> > assumes a variable to refer to a different value, not noticing that it has
> been
> > shadowed. There might be situations though, were it is hard to find a good
> name
> > for a variable, and the addional requirement for the name to be unique,
might
> > encourage usage of bad names. So I might reconsider this rule, if you show
me
> a
> > better example where this rule is problematic.
> 
> Yea, you're probably right.
> 
> While the rule can be annoying it certainly can stop some tricky bugs too. I
> can't see much better an example than I gave already. The rule is kind of
> annoying to follow in places like removeFilter[1] in
> adblockpluscore/lib/filterStorage.js but I don't think that justifies leaving
> the rule off.
> 
> [1] -
>
https://github.com/adblockplus/adblockpluscore/blob/master/lib/filterStorage....

Well, this is also an example, where shadowing makes the code hard to get right.
Without carefully reading the whole code, you might easily overlook the
shadowing and wrongly assume that "subscription" or "position" refers to the
argument initially passed to the function. But I also agree that it might not be
easy coming up with a good distinct variable name here. I guess I personally
might go with abbreviations for the temporary variables (e.g. "sub" and "pos"),
alternatively you could add a prefix (like "thisSubscription"). Either way, this
seems to be not any worse than the side effects of shadowing, and if you really
want to reuse the variable name, you could still just remove the let statement.

[Thomas Greiner, 2017-02-13 16:06:11]

On 2017/02/13 13:13:15, kzar wrote:
> But any opinion on inconsistent braces in an if...else... like this?
> 
> if (true)
>   console.log(1);
> else
> {
>   let a = 1;
>   console.log(a);
> }

No opinion on that because allowing blocks without braces is inconsistent no
matter what. After all, JavaScript requires braces in some places unlike other
languages.

[Felix Dahlke, 2017-02-14 10:26:15]

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29374944/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/13 13:06:13, Thomas Greiner wrote:
> > I don't have any links to hand but IIRC both Thomas and Wladimir told me to
> use braces for if...else... clauses consistently in codereviews in the past.
> 
> I can only speak for myself but my objection was only against multi-line
blocks
> without braces to avoid potential issues when touching them in the future.
> 
> e.g.
> for (let a of b)
>   if (a == c)
>     c = 1;
>   else
>     c = 2;
> 
> No opinion on single-line blocks without braces though.

Quickly asked Wladimir about this: He feels the same as Thomas, in the case
above there should be braces around the for. As for the "consistent" rule, he
doesn't care much. So I suggest we leave it out in the initial version. We can
always add it later if we feel that becomes necessary, but so far nobody seems
to feel strong about it.

[Wladimir Palant, 2017-02-14 12:43:21]

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:11: (As root, or using sudo.)
Nit: I'm not a big fan of having an entire sentence in parentheses (yes, Dave
disagrees). How about:

  This command requires administrator privileges so you might need to use sudo.

No "root" (which a Unix detail) and no parentheses.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:15: To lint a JavaScript file using ESLint you
simply run the `eslint` command with
Nit: Please avoid using the word "simply" in instructions. You don't want to
make people feel stupid if it turns out complicated in the end ;)

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:20: (For advanced usage see `eslint --help`.)
Nit: Parentheses are unnecessary here.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:32: the `"root": true` section from the above
example.)
Nit: Parentheses are unnecessary here.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
As Felix mentioned already, I do not really consider "consistent" necessary
here, nor does it match our current programming style. Since nobody seems to
feel strongly about this, I suggest we remove it.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:56: "no-empty": ["error", {allowEmptyCatch: true}],
For reference: the idea is that empty catch blocks have at least a comment
explaining what kind of errors is expected there and why we aren't handling
them. So IMHO the goal should be to remove allowEmptyCatch here again - but we
will need to remove this pattern from the existing codebase first.

[kzar, 2017-02-15 05:24:23]

Patch Set 12 : Addressed Wladimir's feedback

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:11: (As root, or using sudo.)
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> Nit: I'm not a big fan of having an entire sentence in parentheses (yes, Dave
> disagrees). How about:
> 
>   This command requires administrator privileges so you might need to use
sudo.
> 
> No "root" (which a Unix detail) and no parentheses.

(Done.)

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:15: To lint a JavaScript file using ESLint you
simply run the `eslint` command with
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> Nit: Please avoid using the word "simply" in instructions. You don't want to
> make people feel stupid if it turns out complicated in the end ;)

Good point, Done.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:20: (For advanced usage see `eslint --help`.)
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> Nit: Parentheses are unnecessary here.

Done.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/README.md:32: the `"root": true` section from the above
example.)
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> Nit: Parentheses are unnecessary here.

Done.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:38: "curly": ["error", "multi-or-nest",
"consistent"],
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> As Felix mentioned already, I do not really consider "consistent" necessary
> here, nor does it match our current programming style. Since nobody seems to
> feel strongly about this, I suggest we remove it.

Done.

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:56: "no-empty": ["error", {allowEmptyCatch: true}],
On 2017/02/14 12:43:20, Wladimir Palant wrote:
> For reference: the idea is that empty catch blocks have at least a comment
> explaining what kind of errors is expected there and why we aren't handling
> them. So IMHO the goal should be to remove allowEmptyCatch here again - but we
> will need to remove this pattern from the existing codebase first.

Fair enough, I don't mind if empty catch blocks are allowed when they're
commented. Done.

[Wladimir Palant, 2017-02-15 08:06:10]

I didn't mean that you should remove allowEmptyCatch already but I don't mind
either. LGTM

[Sebastian Noack, 2017-02-15 10:35:25]

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:56: "no-empty": ["error", {allowEmptyCatch: true}],
On 2017/02/15 05:24:22, kzar wrote:
> On 2017/02/14 12:43:20, Wladimir Palant wrote:
> > For reference: the idea is that empty catch blocks have at least a comment
> > explaining what kind of errors is expected there and why we aren't handling
> > them. So IMHO the goal should be to remove allowEmptyCatch here again - but
we
> > will need to remove this pattern from the existing codebase first.
> 
> Fair enough, I don't mind if empty catch blocks are allowed when they're
> commented. Done.


I also prefer to keep the allowEmptyCatch option (for now). FWIW, I'm not even
convinced that every empty catch block should have a comment. Sometimes it seems
quite obvious what error is expected and why it is ignored. And I'm generally
opposed to add redundant comments that doesn't add any information that aren't
already clear from the source code.

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
eslint-config-eyeo/README.md:11: This command requires administrator privileges
so you might need to use sudo.
Nit: While you wrapped other commands in backticks, it seems you forgot to do so
for "sudo", here.

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
eslint-config-eyeo/index.js:53: "no-console": ["error", {allow: ["warn",
"error", "trace"]}],
We discussed that before, but I just noticed that you ignored my last comment on
that debate in the other review. Do we have any legit use of console.warn() and
console.trace() in our code? In particular console.trace() seems to be mostly
useful for debugging, with a similar chance as console.log() to be added
temporarily and then being forgotten.

[kzar, 2017-02-15 11:45:20]

Patch Set 13 : Addressed Sebastian's feedback

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375587/eslint-config-eyeo/...
eslint-config-eyeo/index.js:56: "no-empty": ["error", {allowEmptyCatch: true}],
On 2017/02/15 10:35:24, Sebastian Noack wrote:
> On 2017/02/15 05:24:22, kzar wrote:
> > On 2017/02/14 12:43:20, Wladimir Palant wrote:
> > > For reference: the idea is that empty catch blocks have at least a comment
> > > explaining what kind of errors is expected there and why we aren't
handling
> > > them. So IMHO the goal should be to remove allowEmptyCatch here again -
but
> we
> > > will need to remove this pattern from the existing codebase first.
> > 
> > Fair enough, I don't mind if empty catch blocks are allowed when they're
> > commented. Done.
> 
> 
> I also prefer to keep the allowEmptyCatch option (for now). FWIW, I'm not even
> convinced that every empty catch block should have a comment. Sometimes it
seems
> quite obvious what error is expected and why it is ignored. And I'm generally
> opposed to add redundant comments that doesn't add any information that aren't
> already clear from the source code.

OK, since Wladimir didn't mind either way and you prefer this I've added it back
again.

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
File eslint-config-eyeo/README.md (right):

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
eslint-config-eyeo/README.md:11: This command requires administrator privileges
so you might need to use sudo.
On 2017/02/15 10:35:24, Sebastian Noack wrote:
> Nit: While you wrapped other commands in backticks, it seems you forgot to do
so
> for "sudo", here.

Done.

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
eslint-config-eyeo/index.js:53: "no-console": ["error", {allow: ["warn",
"error", "trace"]}],
On 2017/02/15 10:35:24, Sebastian Noack wrote:
> We discussed that before, but I just noticed that you ignored my last comment
on
> that debate in the other review. Do we have any legit use of console.warn()
and
> console.trace() in our code? In particular console.trace() seems to be mostly
> useful for debugging, with a similar chance as console.log() to be added
> temporarily and then being forgotten.

console.trace is used here:
https://github.com/adblockplus/adblockpluschrome/blob/master/lib/compat.js#L83

As for console.warn I don't see that in any of our code, but we should probably
use it in this code that currently uses console.log:
https://github.com/adblockplus/adblockpluscore/blob/master/lib/rsa.js#L143

[Sebastian Noack, 2017-02-15 11:58:06]

LGTM

(But as I said before, I only agree to no-unref rule under the condition that we
significantly reduce the usage of global variables in adblockpluschrome, rather
than adding much boilerplate there)

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
File eslint-config-eyeo/index.js (right):

https://codereview.adblockplus.org/29374555/diff/29375725/eslint-config-eyeo/...
eslint-config-eyeo/index.js:53: "no-console": ["error", {allow: ["warn",
"error", "trace"]}],
On 2017/02/15 11:45:20, kzar wrote:
> On 2017/02/15 10:35:24, Sebastian Noack wrote:
> > We discussed that before, but I just noticed that you ignored my last
comment
> on
> > that debate in the other review. Do we have any legit use of console.warn()
> and
> > console.trace() in our code? In particular console.trace() seems to be
mostly
> > useful for debugging, with a similar chance as console.log() to be added
> > temporarily and then being forgotten.
> 
> console.trace is used here:
> https://github.com/adblockplus/adblockpluschrome/blob/master/lib/compat.js#L83
> 
> As for console.warn I don't see that in any of our code, but we should
probably
> use it in this code that currently uses console.log:
> https://github.com/adblockplus/adblockpluscore/blob/master/lib/rsa.js#L143

Acknowledged.