Issue 3692 - Add base ESLint configuration

eslint-config-eyeo/index.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2017 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* eslint-env commonjs */
+
+"use strict";
+
+module.exports = {
+  extends: "eslint:recommended",
+  env: {
+    es6: true
+  },
+  rules: {
+    "array-bracket-spacing": "error",
+    "arrow-spacing": "error",
+    "block-scoped-var": "error",
+    "block-spacing": "error",
+    "brace-style": ["error", "allman", {allowSingleLine: true}],
+    "camelcase": ["error", {properties: "never"}],
+    "comma-dangle": "error",
+    "comma-spacing": "error",
+    "comma-style": "error",
+    "computed-property-spacing": "error",
+    "curly": ["error", "multi-or-nest", "consistent"],

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

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]

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:29]

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.

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

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:21]

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

[Thomas Greiner, 2017/02/13 13:06:13]

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.

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

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.

+    "eol-last": "error",
+    "func-call-spacing": "error",
+    "indent": ["error", 2, {SwitchCase: 1, ArrayExpression: "first"}],
+    "key-spacing": "error",
+    "keyword-spacing": "error",
+    "linebreak-style": "error",
+    "lines-around-directive": "error",
+    "max-len": ["error", 80, {ignoreUrls: true}],
+    "new-parens": "error",
+    "no-array-constructor": "error",
+    "no-caller": "error",
+    "no-case-declarations": "off",
+    "no-catch-shadow": "error",
+    "no-cond-assign": "off",
+    "no-console": ["error", {allow: ["warn", "error", "trace"]}],
+    "no-control-regex": "off",
+    "no-else-return": "error",
+    "no-eval": "error",
+    "no-extra-bind": "error",
+    "no-extra-label": "error",
+    "no-implied-eval": "error",
+    "no-labels": ["error", {allowLoop: true}],
+    "no-lone-blocks": "error",
+    "no-lonely-if": "error",
+    "no-multi-spaces": "error",
+    "no-new-func": "error",
+    "no-new-object": "error",
+    "no-proto": "error",
+    "no-self-compare": "error",
+    "no-shadow": "error",

[kzar, 2017/02/13 08:37:25]

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...

[Sebastian Noack, 2017/02/13 11:31:29]

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.

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

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....

[Sebastian Noack, 2017/02/13 14:20:28]

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.

+    "no-trailing-spaces": "error",
+    "no-unneeded-ternary": "error",
+    "no-unused-vars": "off",
+    "no-useless-computed-key": "error",
+    "no-useless-concat": "error",
+    "no-useless-escape": "error",
+    "no-useless-return": "error",
+    "no-var": "error",
+    "no-warning-comments": "error",
+    "no-whitespace-before-property": "error",
+    "no-with": "error",
+    "object-curly-spacing": "error",
+    "object-shorthand": ["error", "always", {
+      avoidExplicitReturnArrows: true
+    }],
+    "one-var": ["error", "never"],
+    "operator-assignment": "error",
+    "operator-linebreak": "error",
+    "padded-blocks": ["error", "never"],
+    "prefer-arrow-callback": "error",
+    "prefer-destructuring": ["error", {array: false}],
+    "prefer-numeric-literals": "error",
+    "prefer-rest-params": "error",
+    "prefer-spread": "error",
+    "quote-props": ["error", "consistent-as-needed"],
+    "quotes": ["error", "double", {avoidEscape: true}],
+    "radix": "error",
+    "rest-spread-spacing": "error",
+    "semi": "error",
+    "semi-spacing": "error",
+    "space-before-function-paren": ["error", "never"],
+    "space-in-parens": "error",
+    "space-infix-ops": "error",
+    "space-unary-ops": "error",
+    "spaced-comment": "error",
+    "strict": ["error", "global"],
+    "valid-jsdoc": ["error", {
+      requireParamDescription: false,
+      requireReturn: false,
+      requireReturnDescription: false
+    }],
+    "yield-star-spacing": "error",
+    "yoda": "error"
+  }
+};