Issue 3692 - Created global eslintrc

See https://codereview.adblockplus.org/29374555/ for continuation.

[juliandoucette, 2016-06-15 15:27:44]

On 2016/06/15 15:17:45, juliandoucette wrote:

I used trev's eslint from easypasswords as a base and modified it for web
projects for use in abb-ios-prototypes. I don't think it would be productive to
review and debate each config property. Instead, I suggest we start using these,
and modify them whenever we run into an issue (error where it shouldn't be, or
no error where error should be).

[Wladimir Palant, 2016-06-15 15:36:28]

As I mentioned in the issue, we shouldn't be duplicating this code. Globals are
better specified via inline configuration in the particular files. Besides, we
should be using the environment setting rather than specify window & co.
explicitly.

[juliandoucette, 2016-06-15 15:49:40]

On 2016/06/15 15:36:28, Wladimir Palant wrote:
> As I mentioned in the issue, we shouldn't be duplicating this code. Globals
are
> better specified via inline configuration in the particular files. Besides, we
> should be using the environment setting rather than specify window & co.
> explicitly.

I agree. 

In that case, ES6 specific rules, like "no-var", will need to be turned off or
ignored inline often. Do you have a preference on this matter?

[juliandoucette, 2016-06-15 15:51:23]

On 2016/06/15 15:49:40, juliandoucette wrote:
> On 2016/06/15 15:36:28, Wladimir Palant wrote:
> > As I mentioned in the issue, we shouldn't be duplicating this code. Globals
> are
> > better specified via inline configuration in the particular files. Besides,
we
> > should be using the environment setting rather than specify window & co.
> > explicitly.
> 
> I agree. 
> 
> In that case, ES6 specific rules, like "no-var", will need to be turned off or
> ignored inline often. Do you have a preference on this matter?

inline or in-project*

[juliandoucette, 2016-06-20 19:20:50]

On 2016/06/15 15:51:23, juliandoucette wrote:
> On 2016/06/15 15:49:40, juliandoucette wrote:
> > On 2016/06/15 15:36:28, Wladimir Palant wrote:
> > > As I mentioned in the issue, we shouldn't be duplicating this code.
Globals
> > are
> > > better specified via inline configuration in the particular files.
Besides,
> we
> > > should be using the environment setting rather than specify window & co.
> > > explicitly.
> > 
> > I agree. 
> > 
> > In that case, ES6 specific rules, like "no-var", will need to be turned off
or
> > ignored inline often. Do you have a preference on this matter?
> 
> inline or in-project*

Scratch that. I will create a base config for ES5 and ES6 and then a version for
ES6 only that extends the base.

[juliandoucette, 2016-06-25 18:44:36]

Ready for review.

[juliandoucette, 2016-09-15 15:53:31]

On 2016/06/25 18:44:36, juliandoucette wrote:
> Ready for review.

Updated reviewers based on last Frontend Developer meeting.

[Wladimir Palant, 2016-09-16 08:07:42]

On 2016/06/15 15:49:40, juliandoucette wrote:
> In that case, ES6 specific rules, like "no-var", will need to be turned off or
> ignored inline often. Do you have a preference on this matter?

Personally, I'd prefer using transpilers when necessary - always write proper
ES6 code, have it dumbed down automatically when deploying to production.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:12: "eol-last": "warn",
This is already part of eslint:recommended, we don't need to duplicate it. If
the point was turning an error into a warning - I don't see why we should,
people tend to ignore warnings.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:13: "vars-on-top": "off",
This isn't part of eslint:recommended, no point switching it off.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:15: "no-irregular-whitespace": "warn",
Like above, this is already part of eslint:recommended.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:20: "no-eq-null": "off",
This isn't part of eslint:recommended, no point switching it off.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:21: "no-redeclare": "error",
This is already part of eslint:recommended.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:23: "no-undef": "error",
This is already part of eslint:recommended.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:24: "no-unused-vars": "warn",
The reason I switched this rule off: it produces tons of false positives. For
example, with your settings `new Promise((resolve, reject) => resolve(true))`
will warn that `reject` is unused. I don't consider leaving out parameters that
are being passed in but not used a good measure.

[kzar, 2016-09-16 10:33:51]

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:4: /* MDN General practices */
(It surprised me to see comments in a JSON file but it looks like eslint strips
them out
http://eslint.org/docs/user-guide/configuring#comments-in-configuration-files )

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:8: "no-alert": "warn",
There are some valid uses of alert in adblockpluschrome at least, but I can't
think of any valid uses of the other three above. Perhaps they should instead be
considered errors?

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:9: "linebreak-style": ["warn", "unix"],
Shouldn't linebreak-style be an error too, or am I missing a valid use case?

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:15: "no-irregular-whitespace": "warn",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> Like above, this is already part of eslint:recommended.

Yea and it should really be an error. One time a zero-width space character did
break some of our code! It was Python code so not totally relevant, but still
scary. It was invisible in Rietveld so passed straight through review!

I set up emacs to highlight those red after that, if you're interested in doing
that too here's how:
https://github.com/kzar/emacs.d/blob/master/init.el#L181-L191

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:21: "no-redeclare": "error",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> This is already part of eslint:recommended.

To play devil's advocate perhaps it's good to specify everything explicitly?
Then should eslint:recommended change in the future we won't be surprised. (If
two developers had different versions it's possible the two defaults could clash
too.)

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:22: "consistent-return": "error",
Does everyone agree with consistent-return? To me it seems kind of verbose to
add a `return undefined;` to the end of functions when that happens implicitly.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:24: "no-unused-vars": "warn",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> The reason I switched this rule off: it produces tons of false positives. For
> example, with your settings `new Promise((resolve, reject) => resolve(true))`
> will warn that `reject` is unused. I don't consider leaving out parameters
that
> are being passed in but not used a good measure.

Agreed, it's better to not use a parameter than silently drop it.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:29: /* Palant recommends */
Nit: Mind adding a newline before each new section?

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:39: "no-var": "warn",
We need to use var in adblockpluschrome in some places, for scripts that aren't
transpiled like content scripts.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:46: "prefer-spread": "warn",
I don't mind using the spread operator instead of .apply when possible but that
would require some changes to our tooling for adblockpluschrome at least.
(Perhaps we could improve jshydra to support those?)

[Wladimir Palant, 2016-09-16 11:37:06]

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:8: "no-alert": "warn",
On 2016/09/16 10:33:50, kzar wrote:
> There are some valid uses of alert in adblockpluschrome at least, but I can't
> think of any valid uses of the other three above. Perhaps they should instead
be
> considered errors?

Actually, using console.error() can be valid when exceptions need to be
reported. In fact, the console object is used several times in adblockpluschrome
repository - all perfectly valid use cases.

So, how are we going to deal with this?

1) Just leave it as is, it's "only" warnings - not a good idea IMHO, as it
trains people to ignore warnings. Ideally, no warnings or errors should be
produced unless there is a real issue.

2) Override the warnings in all the relevant code places - in adblockpluschrome
that's two files using alert() and six files using console. IMHO, too many
exceptions make a rule questionable.

3) Drop these rules and leave it to reviewers to spot debugging code (which
could be using Cu.reportError() or dump() as well).

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:21: "no-redeclare": "error",
On 2016/09/16 10:33:51, kzar wrote:
> To play devil's advocate perhaps it's good to specify everything explicitly?
> Then should eslint:recommended change in the future we won't be surprised.

I think that eslint:recommended usually changes when eslint's capabilities
change, so we'll have to adjust one way or another. eslint:recommended actually
makes it easier to catch up here.

It definitely makes things easier that our rules are based on a "common
standard" and we merely document deviations from it. If somebody wants to learn
everything important about our coding style - it's only going to be a few dozen
rules and not a few hundred.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:22: "consistent-return": "error",
On 2016/09/16 10:33:51, kzar wrote:
> Does everyone agree with consistent-return? To me it seems kind of verbose to
> add a `return undefined;` to the end of functions when that happens
implicitly.

Yes, consistent-return is important and actually something we already do
consistently to my knowledge. I cannot imagine a situation where you would have
`return undefined` at the end of a function, maybe `return null` (which already
isn't equivalent to not returning anything). And it is a very common mistake to
forget returning a value on one of the branches of the function.

Note that this doesn't require you having an explicit return at the end of the
function if none of its branches return a value.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:39: "no-var": "warn",
On 2016/09/16 10:33:51, kzar wrote:
> We need to use var in adblockpluschrome in some places, for scripts that
aren't
> transpiled like content scripts.

This is an ES6 config, and we are already discussing what we should do with ES5
environments. Overriding this rule in a file is definitely possible, but
personally I'd favor just transpiling everything.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:46: "prefer-spread": "warn",
On 2016/09/16 10:33:51, kzar wrote:
> (Perhaps we could improve jshydra to support those?)

We definitely could and should.

Note that we are quickly getting to the point where transpiling in extensions is
only necessary to support Safari. Chrome 49 and Edge support all ES6 features we
need.

[kzar, 2016-09-16 12:12:58]

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:8: "no-alert": "warn",
On 2016/09/16 11:37:06, Wladimir Palant wrote:
> On 2016/09/16 10:33:50, kzar wrote:
> > There are some valid uses of alert in adblockpluschrome at least, but I
can't
> > think of any valid uses of the other three above. Perhaps they should
instead
> be
> > considered errors?
> 
> Actually, using console.error() can be valid when exceptions need to be
> reported. In fact, the console object is used several times in
adblockpluschrome
> repository - all perfectly valid use cases.
> 
> So, how are we going to deal with this?
> 
> 1) Just leave it as is, it's "only" warnings - not a good idea IMHO, as it
> trains people to ignore warnings. Ideally, no warnings or errors should be
> produced unless there is a real issue.
> 
> 2) Override the warnings in all the relevant code places - in
adblockpluschrome
> that's two files using alert() and six files using console. IMHO, too many
> exceptions make a rule questionable.
> 
> 3) Drop these rules and leave it to reviewers to spot debugging code (which
> could be using Cu.reportError() or dump() as well).

Hmm good point. I guess I'm easy between options 2 and 3, but I agree about 1
being a bad idea.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:21: "no-redeclare": "error",
On 2016/09/16 11:37:06, Wladimir Palant wrote:
> On 2016/09/16 10:33:51, kzar wrote:
> > To play devil's advocate perhaps it's good to specify everything explicitly?
> > Then should eslint:recommended change in the future we won't be surprised.
> 
> I think that eslint:recommended usually changes when eslint's capabilities
> change, so we'll have to adjust one way or another. eslint:recommended
actually
> makes it easier to catch up here.
> 
> It definitely makes things easier that our rules are based on a "common
> standard" and we merely document deviations from it. If somebody wants to
learn
> everything important about our coding style - it's only going to be a few
dozen
> rules and not a few hundred.

Fair enough.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:46: "prefer-spread": "warn",
On 2016/09/16 11:37:06, Wladimir Palant wrote:
> On 2016/09/16 10:33:51, kzar wrote:
> > (Perhaps we could improve jshydra to support those?)
> 
> We definitely could and should.
> 
> Note that we are quickly getting to the point where transpiling in extensions
is
> only necessary to support Safari. Chrome 49 and Edge support all ES6 features
we
> need.

Perhaps I'll give it a go if I ever find time. I very briefly looked at it
before but wasn't sure how to do it without binding the function. That's
offtopic here anyway so I've opened an issue
https://issues.adblockplus.org/ticket/4443

[juliandoucette, 2016-09-16 12:42:13]

> If the point was turning an error into a warning - I don't see why we should,
people tend to ignore warnings.

I intended to use "error" when the issue very likely results in a JS error and
"warning" everywhere else.

I find it annoying that eslint exits after encountering an "error".

PS: I'm hoping that you guys will review **and** submit changesets on this
issue.

[kzar, 2016-09-16 14:05:22]

On 2016/09/16 12:42:13, juliandoucette wrote:
> PS: I'm hoping that you guys will review **and** submit changesets on this
> issue.

Is it even possible to submit patch sets to someone else's review? (I always
assumed it wasn't, but never tried.)

[juliandoucette, 2016-09-16 15:09:25]

> Is it even possible to submit patch sets to someone else's review? (I always
> assumed it wasn't, but never tried.)

I don't know. Give it a try here :) .

[Wladimir Palant, 2016-09-19 12:28:34]

On 2016/09/16 15:09:25, juliandoucette wrote:
> > Is it even possible to submit patch sets to someone else's review? (I always
> > assumed it wasn't, but never tried.)
> 
> I don't know. Give it a try here :) .

No, by default other people cannot submit changes. You have to edit issue
description and add something like "COLLABORATOR=somebody@adblockplus.org" - a
well-hidden and undocumented Rietveld feature.

[juliandoucette, 2016-09-26 18:37:40]

> No, by default other people cannot submit changes. You have to edit issue
> description and add something like
mailto:"COLLABORATOR=somebody@adblockplus.org" - a
> well-hidden and undocumented Rietveld feature.

I've added all reviewers as contributors.

[juliandoucette, 2016-10-13 14:14:10]

I think I've covered everyones comments so far.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:4: /* MDN General practices */
On 2016/09/16 10:33:51, kzar wrote:
> (It surprised me to see comments in a JSON file but it looks like eslint
strips
> them out
> http://eslint.org/docs/user-guide/configuring#comments-in-configuration-files
)

Acknowledged.

Let me know if you think we should remove them.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:8: "no-alert": "warn",
On 2016/09/16 12:12:57, kzar wrote:
> On 2016/09/16 11:37:06, Wladimir Palant wrote:
> > On 2016/09/16 10:33:50, kzar wrote:
> > > There are some valid uses of alert in adblockpluschrome at least, but I
> can't
> > > think of any valid uses of the other three above. Perhaps they should
> instead
> > be
> > > considered errors?
> > 
> > Actually, using console.error() can be valid when exceptions need to be
> > reported. In fact, the console object is used several times in
> adblockpluschrome
> > repository - all perfectly valid use cases.
> > 
> > So, how are we going to deal with this?
> > 
> > 1) Just leave it as is, it's "only" warnings - not a good idea IMHO, as it
> > trains people to ignore warnings. Ideally, no warnings or errors should be
> > produced unless there is a real issue.
> > 
> > 2) Override the warnings in all the relevant code places - in
> adblockpluschrome
> > that's two files using alert() and six files using console. IMHO, too many
> > exceptions make a rule questionable.
> > 
> > 3) Drop these rules and leave it to reviewers to spot debugging code (which
> > could be using Cu.reportError() or dump() as well).
> 
> Hmm good point. I guess I'm easy between options 2 and 3, but I agree about 1
> being a bad idea.

Acknowledged.

I changed this to rely on eslint:recommends for no-debugger and no-console and
error on "no-alert". 

Let me know if you think there are too many exceptions to this rule.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:9: "linebreak-style": ["warn", "unix"],
On 2016/09/16 10:33:50, kzar wrote:
> Shouldn't linebreak-style be an error too, or am I missing a valid use case?

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:12: "eol-last": "warn",
On 2016/09/16 08:07:42, Wladimir Palant wrote:
> This is already part of eslint:recommended, we don't need to duplicate it. If
> the point was turning an error into a warning - I don't see why we should,
> people tend to ignore warnings.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:13: "vars-on-top": "off",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> This isn't part of eslint:recommended, no point switching it off.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:15: "no-irregular-whitespace": "warn",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> Like above, this is already part of eslint:recommended.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:20: "no-eq-null": "off",
On 2016/09/16 08:07:42, Wladimir Palant wrote:
> This isn't part of eslint:recommended, no point switching it off.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:21: "no-redeclare": "error",
On 2016/09/16 12:12:57, kzar wrote:
> On 2016/09/16 11:37:06, Wladimir Palant wrote:
> > On 2016/09/16 10:33:51, kzar wrote:
> > > To play devil's advocate perhaps it's good to specify everything
explicitly?
> > > Then should eslint:recommended change in the future we won't be surprised.
> > 
> > I think that eslint:recommended usually changes when eslint's capabilities
> > change, so we'll have to adjust one way or another. eslint:recommended
> actually
> > makes it easier to catch up here.
> > 
> > It definitely makes things easier that our rules are based on a "common
> > standard" and we merely document deviations from it. If somebody wants to
> learn
> > everything important about our coding style - it's only going to be a few
> dozen
> > rules and not a few hundred.
> 
> Fair enough.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:22: "consistent-return": "error",
On 2016/09/16 11:37:06, Wladimir Palant wrote:
> On 2016/09/16 10:33:51, kzar wrote:
> > Does everyone agree with consistent-return? To me it seems kind of verbose
to
> > add a `return undefined;` to the end of functions when that happens
> implicitly.
> 
> Yes, consistent-return is important and actually something we already do
> consistently to my knowledge. I cannot imagine a situation where you would
have
> `return undefined` at the end of a function, maybe `return null` (which
already
> isn't equivalent to not returning anything). And it is a very common mistake
to
> forget returning a value on one of the branches of the function.
> 
> Note that this doesn't require you having an explicit return at the end of the
> function if none of its branches return a value.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:23: "no-undef": "error",
On 2016/09/16 08:07:43, Wladimir Palant wrote:
> This is already part of eslint:recommended.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:24: "no-unused-vars": "warn",
On 2016/09/16 10:33:51, kzar wrote:
> On 2016/09/16 08:07:43, Wladimir Palant wrote:
> > The reason I switched this rule off: it produces tons of false positives.
For
> > example, with your settings `new Promise((resolve, reject) =>
resolve(true))`
> > will warn that `reject` is unused. I don't consider leaving out parameters
> that
> > are being passed in but not used a good measure.
> 
> Agreed, it's better to not use a parameter than silently drop it.

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:29: /* Palant recommends */
On 2016/09/16 10:33:50, kzar wrote:
> Nit: Mind adding a newline before each new section?

Done.

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:46: "prefer-spread": "warn",
On 2016/09/16 12:12:57, kzar wrote:
> On 2016/09/16 11:37:06, Wladimir Palant wrote:
> > On 2016/09/16 10:33:51, kzar wrote:
> > > (Perhaps we could improve jshydra to support those?)
> > 
> > We definitely could and should.
> > 
> > Note that we are quickly getting to the point where transpiling in
extensions
> is
> > only necessary to support Safari. Chrome 49 and Edge support all ES6
features
> we
> > need.
> 
> Perhaps I'll give it a go if I ever find time. I very briefly looked at it
> before but wasn't sure how to do it without binding the function. That's
> offtopic here anyway so I've opened an issue
> https://issues.adblockplus.org/ticket/4443

Acknowledged.

[kzar, 2017-01-31 10:17:49]

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29347028/.eslintrc.json#newc...
.eslintrc.json:22: "consistent-return": "error",
On 2016/10/13 14:14:08, juliandoucette wrote:
> On 2016/09/16 11:37:06, Wladimir Palant wrote:
> > On 2016/09/16 10:33:51, kzar wrote:
> > > Does everyone agree with consistent-return? To me it seems kind of verbose
> to
> > > add a `return undefined;` to the end of functions when that happens
> > implicitly.
> > 
> > Yes, consistent-return is important and actually something we already do
> > consistently to my knowledge. I cannot imagine a situation where you would
> have
> > `return undefined` at the end of a function, maybe `return null` (which
> already
> > isn't equivalent to not returning anything). And it is a very common mistake
> to
> > forget returning a value on one of the branches of the function.
> > 
> > Note that this doesn't require you having an explicit return at the end of
the
> > function if none of its branches return a value.
> 
> Done.

One example where I think consistent-return might be a bad idea is the
onHeadersReceived listener in adblockpluschrome/lib/csp.js[1]. There we check if
any blocking filters match, if not we don't do anything (implicitly returning
`undefined`), otherwise we return the headers array with our CSP header pushed
on the end.

Initially I thought the rule was OK, I simply changed the logic around so first
we `return;` if there is no matching blocking filter. Problem was _even that_
wasn't enough for the consistent-return rule, I had to do explicitly `return
undefined;` instead.

IMO that's pretty ugly, I'd rather not litter our code with `return
undefined;`'s and so I'd rather not use the consistent-return rule. What do you
think?

[1] -
https://github.com/adblockplus/adblockpluschrome/blob/master/lib/csp.js#L24-L50

[Sebastian Noack, 2017-01-31 20:36:45]

I had a go running ESLint with the configuration from this review on the code in
adblockpluschrome. I think it's easier to discuss the suggested configuration,
when seeing how it actually behaves with our code. Here is the whole output:
https://gist.github.com/snoack/54b72699bdfb42dbf2fa43abdd284040

However, I first had to add following to the configuration (otherwise ESLint
just fails with a bunch of parser errors due to the use of let and const
statements):

  "parserOptions": {
    "ecmaVersion": 6
  }


I identified following rules, which are in effect, as inappropriate or
problematic at least:

no-undef
--------
There are plenty 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.

consistent-return
-----------------
As Dave explained above, we use a couple APIs that are designed for optional
return values. One example is the webRequest API; if you just want to record
some network events you wouldn't return anything, however if you want to
manipulate how a request is processed (e.g. block it) you have to return data
indicating the desired action. Another example includes Promises; if a callback
attached to a Promise returns a value it is passed down the callback chain, but
if this is the end of the chain there is no point in returning something. I
agree with Dave, that always adding an explicit "return undefined" to the end of
those functions, doesn't make our code any easier to work with.

no-control-regex
----------------
We have a regular expression that matches any character that needs to be escaped
in CSS, that is "\", "{", "}" and everything that isn't in the range of
printable ASCII-characters. The simplest way to do this is with this regular
expression /["\\\{\}\x00-\x1F\x7F]/, which apparently is causing a
no-control-regex error. I don't see anything wrong with that regular expression.
The alternatives are certainly more complex.

no-cond-assign
--------------
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.

no-case-declarations
--------------------
This rule is triggered when you have a function-scoped declaration (e.g. let
statement), after a case statement, without wrapping it into a block. If the
code doesn't "break" before another "case" is reached, the variable (despite
being block-scoped) might be, or not be, defined in the code below. However,
putting any code that does not (or might not) "break" after a case-statement is
something we generally avoid (it has been rejected in code reviews before), as
it is a footgun that easily leads to a wide range of bugs. However, ESLint
doesn't care whether potentially another "case" can be reached (or whether the
variable is potentially used again). So in the cases I've seen, this error
wasn't about a potentially error-prone practice, but simply about ESLint being
too naive. And I'm opposed to add additional boilerplate to our code just to
workaround a linter that is trying to be too smart.

no-var
------
While we can now use block-scoping in all of our browser extension code, and
updated our coding practices to do so wherever possible, there is still one
scenario in which we have to resort to the var-statement, even where legacy
browsers are no concern. That is when defining global variables that has to be
accessed by other scripts. Generally, we wrap a script's code into a block (done
automatically during the build process for modules) to avoid leaking internal
variables. So the only way to make a variable global is using the let statement.
I suppose when Julian tested the configuration, this rule wasn't in effect since
he had ES6 disabled (see above). Otherwise, it might also cause conflicts with
our website code, which has still has to be compatible with some browsers that
don't support block-scoping yet.

no-alert
--------
We have 6 calls to alert()/confirm() in adblockpluschrome. While this might not
be the most elegant way to interact with the user, this is rather a user design
issue, not a code quality issue. In the context of browser extensions, there
might even be legit use cases for alert() and confirm(), e.g. when there is no
user interface. Anyway, using alert() or confirm() isn't something that usually
happens accidentally and somebody would change their mind about because of a
linter warning.

no-console
----------
We also have some legit occurrences of console.error() in adblockpluschrome.
That is when handling unexpected errors, there we want to make sure that they
still show up in the background page's log, for debugging purposes.

indent
------
While it found some instances of indentation that are arguably inconsistent, it
seems at least the way it expects case-statements to be indented is wrong. In
the example below it complains about the case statement to be indented two
spaces too much:
  
  switch (foo)
  {
    case "bar":
    ...
  }

brace-style
-----------
Currently, we put the opening and closing brace on the same line, when defining
empty functions, and for one-liner arrow functions that require braces, causing
a brace-style error. We could put this practice up for discussion, though.

object-curly-spacing
--------------------
Usually, we don't add an extra space after the opening brace, or before the
closing brace, of an object literal. But there are some instances where we do
add the extra space, and officially this is how we are supposed do it, according
to Mozilla's coding practices which we refer to in our coding practices. So
while I'm fine with this rule, I think we have to adapt our coding practices
first, before implementing a linter configuration that is in contradiction with
it.

valid-jsdoc
-----------
While we use jsdoc, and having a linter that validates the syntax of the API
documentation seems useful, I feel that ESLint is a bit too strict here.
It requires return values to be documented, even if the function doesn't
explicitly return. Also it requires all documented functions and their arguments
to have a description. However, I think that partial documentation is better
than no documentation or redundant documentation. For now this generates a lot
of noise, and in the long-term this might even discourage (reasonable)
documentation.

[kzar, 2017-02-01 05:23:22]

On 2017/01/31 20:36:45, Sebastian Noack wrote:
> I had a go running ESLint with the configuration from this review on the code
in
> adblockpluschrome. I think it's easier to discuss the suggested configuration,
> when seeing how it actually behaves with our code.

Yea me too, that's exactly what I started doing yesterday. I have made progress
now where adblockpluschrome nearly passes, but I haven't started looking at the
output when it's run on the adblockpluscore, adblockplus or adblockplusui code
yet. I plan to start new reviews once I have something I'm happy with, which
will include the required changes to the other repositories. Hopefully that will
give enough context for a proper discussion.

> no-undef
> --------
> There are plenty 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.

Yes, this rule is kind of a pain but perhaps useful too. I found that when I set
the correct env settings for areas of code, for example `qunit` for the unit
tests this got a lot better. But I did find I had to sometimes add /* global ...
*/ comments to the top of files too.

So far I'm undecided on this rule.

> consistent-return
> -----------------
> As Dave explained above, we use a couple APIs that are designed for optional
> return values. One example is the webRequest API; if you just want to record
> some network events you wouldn't return anything, however if you want to
> manipulate how a request is processed (e.g. block it) you have to return data
> indicating the desired action. Another example includes Promises; if a
callback
> attached to a Promise returns a value it is passed down the callback chain,
but
> if this is the end of the chain there is no point in returning something. I
> agree with Dave, that always adding an explicit "return undefined" to the end
of
> those functions, doesn't make our code any easier to work with.

Yea I'd rather just remove this rule.

> no-control-regex
> ----------------
> We have a regular expression that matches any character that needs to be
escaped
> in CSS, that is "\", "{", "}" and everything that isn't in the range of
> printable ASCII-characters. The simplest way to do this is with this regular
> expression /["\\\{\}\x00-\x1F\x7F]/, which apparently is causing a
> no-control-regex error. I don't see anything wrong with that regular
expression.
> The alternatives are certainly more complex.

Agreed, I plan to remove this rule too.

> no-cond-assign
> --------------
> 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.

Well I agree that this rule is _sometimes_ a pain but so far I only found one
place in adblockpluschore where we violate it. For now I've added an inline
ESLint exception comment for the one instance but otherwise left the rule
globally active. I'm undecided if that's better than just removing the rule
globally.

> no-case-declarations
> --------------------
> This rule is triggered when you have a function-scoped declaration (e.g. let
> statement), after a case statement, without wrapping it into a block. If the
> code doesn't "break" before another "case" is reached, the variable (despite
> being block-scoped) might be, or not be, defined in the code below. However,
> putting any code that does not (or might not) "break" after a case-statement
is
> something we generally avoid (it has been rejected in code reviews before), as
> it is a footgun that easily leads to a wide range of bugs. However, ESLint
> doesn't care whether potentially another "case" can be reached (or whether the
> variable is potentially used again). So in the cases I've seen, this error
> wasn't about a potentially error-prone practice, but simply about ESLint being
> too naive. And I'm opposed to add additional boilerplate to our code just to
> workaround a linter that is trying to be too smart.

Yea as discussed in IRC I plan to remove this rule.

> no-var
> ------
> While we can now use block-scoping in all of our browser extension code, and
> updated our coding practices to do so wherever possible, there is still one
> scenario in which we have to resort to the var-statement, even where legacy
> browsers are no concern. That is when defining global variables that has to be
> accessed by other scripts. Generally, we wrap a script's code into a block
(done
> automatically during the build process for modules) to avoid leaking internal
> variables. So the only way to make a variable global is using the let
statement.

There's only a few places where we still use `var` in adblockpluschrome and I
wouldn't mind ESLint exception comments to them. My reasoning is that we should
already have a comment nearby to anyway to explain why `var` was necessary, so
it's not much worse to add the ESLint hint while at it.

> I suppose when Julian tested the configuration, this rule wasn't in effect
since
> he had ES6 disabled (see above). Otherwise, it might also cause conflicts with
> our website code, which has still has to be compatible with some browsers that
> don't support block-scoping yet.

We already agreed this ESLint configuration was only for modern JavaScript
codebases and more recently I've updated the issue description to be clear about
exactly which repositories we're targeting (adblockpluschrome, adblockplus,
adblockpluscore and adblockplusui) here.

> no-console
> ----------
> ...
> no-alert
> --------
> ...

Yep I agree about these rules, removed those.

> indent
> ------
> While it found some instances of indentation that are arguably inconsistent,
it
> seems at least the way it expects case-statements to be indented is wrong. In
> the example below it complains about the case statement to be indented two
> spaces too much:
>   
>   switch (foo)
>   {
>     case "bar":
>     ...
>   }

Yep, I already fixed the rule for switch statements. (I also played with some of
the other options like CallExpression and FunctionExpression but I didn't find
any available settings which suited how we did things so I left them disabled.)

> brace-style
> -----------
> Currently, we put the opening and closing brace on the same line, when
defining
> empty functions, and for one-liner arrow functions that require braces,
causing
> a brace-style error. We could put this practice up for discussion, though.

Yep, there's an allowSingleLine option for the brace-style rule which I've now
enabled.

> object-curly-spacing
> --------------------
> Usually, we don't add an extra space after the opening brace, or before the
> closing brace, of an object literal. But there are some instances where we do
> add the extra space, and officially this is how we are supposed do it,
according
> to Mozilla's coding practices which we refer to in our coding practices. So
> while I'm fine with this rule, I think we have to adapt our coding practices
> first, before implementing a linter configuration that is in contradiction
with
> it.

I wondered which was best here too, so I tried running linting with "always" and
then with "never" and compared the results. Turns out there were very few places
where we added the spacing (IIRC I was always the guilty party adding those),
but many where we didn't. Therefore I've left this as the default "never".

I agree we should probably update the coding style guide about this, mind doing
that?

> valid-jsdoc
> -----------
> While we use jsdoc, and having a linter that validates the syntax of the API
> documentation seems useful, I feel that ESLint is a bit too strict here.
> It requires return values to be documented, even if the function doesn't
> explicitly return. Also it requires all documented functions and their
arguments
> to have a description. However, I think that partial documentation is better
> than no documentation or redundant documentation. For now this generates a lot
> of noise, and in the long-term this might even discourage (reasonable)
> documentation.

I agree, I tweaked some of the options so you don't have to document the return
value if there isn't one and so that some of the descriptions are optional. I
left the rule on however since it _did_ find a few real problems which IMO is
useful since the JSDoc syntax can be a little tricky to get right.

[kzar, 2017-02-01 06:32:44]

On 2017/02/01 05:23:22, kzar wrote:
> On 2017/01/31 20:36:45, Sebastian Noack wrote:
> > no-undef
> > -------- 
> > ...
> ...
> So far I'm undecided on this rule.

I think I'd like to remove the no-undef rule now. It's an increasingly annoying,
the more that I work I do on this issue the more inconvenient the rule gets.
Perhaps it makes sense for websites but IMO not really for extension
development.

[Sebastian Noack, 2017-02-01 10:04:47]

On 2017/02/01 06:32:44, kzar wrote:
> On 2017/02/01 05:23:22, kzar wrote:
> > On 2017/01/31 20:36:45, Sebastian Noack wrote:
> > > no-undef
> > > -------- 
> > > ...
> > ...
> > So far I'm undecided on this rule.
> 
> I think I'd like to remove the no-undef rule now. It's an increasingly
annoying,
> the more that I work I do on this issue the more inconvenient the rule gets.
> Perhaps it makes sense for websites but IMO not really for extension
> development.

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

> > no-cond-assign
> > --------------
> > 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.
> 
> Well I agree that this rule is _sometimes_ a pain but so far I only found one
> place in adblockpluschore where we violate it. For now I've added an inline
> ESLint exception comment for the one instance but otherwise left the rule
> globally active. I'm undecided if that's better than just removing the rule
> globally.

Well, so far we might only have one instance where this rule is triggered,
discouraging reasonable code. But so far it didn't found any questionable usage
of assignment in a conditional statement which made it past our code review. So
I'd rather have this rule disabled, than adding a bogus comment, and potentially
discouraging that pattern in the future. It seems they even admit assignment to
be legit in loop statements, as this rule apparently would allow it if you wrap
the assignment in redundant parentheses, according to the ESLint documentation.
But that isn't any less bogus than adding a comment. 

> > no-var
> > ------
> > While we can now use block-scoping in all of our browser extension code, and
> > updated our coding practices to do so wherever possible, there is still one
> > scenario in which we have to resort to the var-statement, even where legacy
> > browsers are no concern. That is when defining global variables that has to
be
> > accessed by other scripts. Generally, we wrap a script's code into a block
> (done
> > automatically during the build process for modules) to avoid leaking
internal
> > variables. So the only way to make a variable global is using the let
> statement.
> 
> There's only a few places where we still use `var` in adblockpluschrome and I
> wouldn't mind ESLint exception comments to them. My reasoning is that we
should
> already have a comment nearby to anyway to explain why `var` was necessary, so
> it's not much worse to add the ESLint hint while at it.

I had a closer look at the instances where we still use the var-statement. As
far as adblockpluschrome is concerned, I think we could even get rid of the
remaining var-statements, by simply moving the declarations to the top of the
respective files, above any block.

> > no-console
> > ----------
> > ...
> > no-alert
> > --------
> > ...
> 
> Yep I agree about these rules, removed those.

I just noticed, we only use console.error() and ESLint apparently can be
configured to specifically only allow particular console methods. That way it
would still catch if you forget about a console.log() that you temporarily added
for debugging.

I completely agree to everything else. I wasn't aware about the per-rule options
when I replied before, but I'm happy to see that we can still use some of those
rules with some tweaks. Good job!

[kzar, 2017-02-01 10:23:52]

On 2017/02/01 10:04:47, Sebastian Noack wrote:
> no-cond-assign
> --------------
> ...
> Well, so far we might only have one instance where this rule is triggered,
> discouraging reasonable code. But so far it didn't found any questionable
usage
> of assignment in a conditional statement which made it past our code review.
So
> I'd rather have this rule disabled, than adding a bogus comment, and
potentially
> discouraging that pattern in the future. It seems they even admit assignment
to
> be legit in loop statements, as this rule apparently would allow it if you
wrap
> the assignment in redundant parentheses, according to the ESLint
documentation.
> But that isn't any less bogus than adding a comment. 

Fair enough, I agree. I've removed that rule now too.

> no-var
> ------
> ...
> I had a closer look at the instances where we still use the var-statement. As
> far as adblockpluschrome is concerned, I think we could even get rid of the
> remaining var-statements, by simply moving the declarations to the top of the
> respective files, above any block.

Good point I'll try that. In any case I'll leave this rule on for now, but let's
see how it holds up after I try running it on the other repositories.
 
> no-console
> ----------
> ...
> I just noticed, we only use console.error() and ESLint apparently can be
> configured to specifically only allow particular console methods. That way it
> would still catch if you forget about a console.log() that you temporarily
added
> for debugging.

Cool idea. Unfortunately it works with a whitelist not blacklist, so I've gone
with this so far:

  "no-console": ["error", {"allow": ["warn", "error", "trace"]}] 

Can you think of any other console methods that should be allowed?

[Sebastian Noack, 2017-02-01 10:51:44]

On 2017/02/01 10:23:52, kzar wrote:
> Cool idea. Unfortunately it works with a whitelist not blacklist, so I've gone
> with this so far:
> 
>   "no-console": ["error", {"allow": ["warn", "error", "trace"]}] 
> 
> Can you think of any other console methods that should be allowed?

I'm not even sure whether it is worth whitelisting "warn" and "trace". I
couldn't find any instance of it in our code. But feel free to double check.
Otherwise, if we need any other console method, for whatever legit purpose, in
the future, I suppose we can just update the ESLint configuration.

[Sebastian Noack, 2017-02-01 19:50:36]

I went through the list of available rules that we don't use yet, and found some
more rules that I'd like to suggest. I briefly tested them on adblockpluschrome.
Most rules aren't even triggered. But some are, and on the first glance I agree
that the code should be changed there. What do you think?

    "block-scoped-var": "error",
    "block-spacing": "error",
    "camelcase": "error",
    "comma-dangle": "error",
    "eol-last": "error",
    "linebreak-style": "error",
    "lines-around-directive": "error",
    "no-array-constructor": "error",
    "no-caller": "error",
    "no-catch-shadow": "error",
    "no-eval": "error",
    "no-extra-bind": "error",
    "no-extra-label": "error",
    "no-implied-eval": "error",
    "no-lone-blocks": "error",
    "no-lonely-if": "error",
    "no-loop-func": "error",
    "no-multi-spaces": "error",
    "no-new-func": "error",
    "no-new-object": "error",
    "no-proto": "error",
    "no-self-compare": "error",
    "no-shadow": "error",
    "no-trailing-spaces": "error",
    "no-unneeded-ternary": "error",
    "no-useless-computed-key": "error",
    "no-useless-concat": "error",
    "no-useless-escape": "error",
    "no-useless-return": "error",
    "no-with": "error",
    "object-shorthand": "error",
    "one-var": ["error", "never"],
    "prefer-arrow-callback": "error",
    "prefer-numeric-literals": "error",
    "quote-props": ["error", "as-needed"],
    "radix": "error",
    "rest-spread-spacing": "error",
    "strict": ["error", "global"],
    "yoda": "error"

https://codereview.adblockplus.org/29346539/diff/29356710/eslintrc.json
File eslintrc.json (right):

https://codereview.adblockplus.org/29346539/diff/29356710/eslintrc.json#newco...
eslintrc.json:11: "vars-on-top": "off",
According to the documentation, it seems vars-on-top isn't included in
eslint:recommended. So might overriding this option be redundant?

https://codereview.adblockplus.org/29346539/diff/29356710/eslintrc.json#newco...
eslintrc.json:27: "no-unused-vars": "off",
I just noticed, instead of completely disabling no-unused-vars, we could rather
configure it to ignore global variables and function arguments, so that it still
catches unused local variables, which I think have no legit use whatsoever:

  "no-unused-vars": ["error", {"vars": "local", "args": "none"}]

https://codereview.adblockplus.org/29346539/diff/29356710/eslintrc.json#newco...
eslintrc.json:45: "require-yield": "error",
It seems this is already included in eslint:recommended. So might overriding
this option be redundant?

[Sebastian Noack, 2017-02-02 13:01:37]

On 2017/02/01 19:50:36, Sebastian Noack wrote:
> I went through the list of available rules that we don't use yet, and found
some
> more rules that I'd like to suggest. I briefly tested them on
adblockpluschrome.
> Most rules aren't even triggered. But some are, and on the first glance I
agree
> that the code should be changed there. What do you think?
> 
>     ...

I revised the list of rules, I suggest to add. Here is the new list:

    "block-scoped-var": "error",
    "block-spacing": "error",
    "comma-dangle": "error",
    "eol-last": "error",
    "linebreak-style": "error",
    "lines-around-directive": "error",
    "no-array-constructor": "error",
    "no-caller": "error",
    "no-catch-shadow": "error",
    "no-eval": "error",
    "no-extra-bind": "error",
    "no-extra-label": "error",
    "no-implied-eval": "error",
    "no-labels": "error",
    "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",
    "no-trailing-spaces": "error",
    "no-unneeded-ternary": "error",
    "no-useless-computed-key": "error",
    "no-useless-concat": "error",
    "no-useless-escape": "error",
    "no-useless-return": "error",
    "no-with": "error",
    "object-shorthand": ["error", "always", {"avoidExplicitReturnArrows":
true}],
    "one-var": ["error", "never"],
    "prefer-arrow-callback": "error",
    "prefer-destructuring": ["error", {"array": false}],
    "prefer-numeric-literals": "error",
    "quote-props": ["error", "as-needed"],
    "radix": "error",
    "rest-spread-spacing": "error",
    "strict": ["error", "global"],
    "yoda": "error"

And here are the required changes to adblockpluschrome in order to make our code
comply to the rules above. I'm quite happy with those changes, as they tidy up
the code a bit. Dave, if you go ahead with those rules, feel free to merge those
changes with the changes you prepared for adblockpluschrome:
https://pastebin.mozilla.org/8972362

Unfortunately, it turned out that we cannot use the camelcase rule, which is a
bummer, as accidental usage of underscores happens all the time. The only
problem here is our Prefs object, as we use underscores for preferences.
Changing the way preferences are stored (which currently is based on their
property names) isn't an option because of backwards compatibility and external
preconfiguration. Also we cannot simply add an eslint comment to the prefs
module where the preferences are defined, as the camelcase rule is not only
triggered by name definitions, but also by name/property lookups. However, what
we could do (and probably should do in the long-term) is changing the Pref
object's properties to use camelcase for consistency, while keep storing them in
the underlying storage with underscores for compatibility. This however, is a
non-trivial effort, as this has to be tackled simultaneously in
adblockpluschrome, adblockplus, adbplockpluscore and adblockplusui.

[kzar, 2017-02-03 08:42:47]

On 2017/02/02 13:01:37, Sebastian Noack wrote:
> I revised the list of rules, I suggest to add. Here is the new list:
> ...

I've just gone through all of those and they all look good to me. I'll add those
all, then go through and figure out which duplicates we have and also check to
see if any are set by default by eslint:recommended.

> And here are the required changes to adblockpluschrome...

Those changes look good to me. Unfortunately they wont apply cleanly, I guess
you made them to master not my branch. No matter anyway I'll go through and
merge them when I get a chance.

> Unfortunately, it turned out that we cannot use the camelcase rule...

Well how about using it but with the extra option so that property names aren't
checked? That mostly passes on adblockpluschrome with just two fixes required to
options.js:

"camelcase": ["error", {"properties": "never"}]

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

On 2017/02/03 08:42:47, kzar wrote:
> I've just gone through all of those and they all look good to me. I'll add
those
> all, then go through and figure out which duplicates we have and also check to
> see if any are set by default by eslint:recommended.

OK, I've now checked for duplicates and for any redundant rules which are on /
off by default. Here's the new review:
https://codereview.adblockplus.org/29374555/

[Sebastian Noack, 2017-02-03 10:18:20]

On 2017/02/03 08:42:47, kzar wrote:
> I've just gone through all of those and they all look good to me. I'll add
those
> all, then go through and figure out which duplicates we have and also check to
> see if any are set by default by eslint:recommended.

There should be no rules that we've added before or are already included in
eslint:recommended, among those I suggested to add. I already checked that.
However, as I pointed out previously, there were some redundancies in the rules
we added before. Anyway, I'm happy that you agree to the rules I suggested to
add.

> Well how about using it but with the extra option so that property names
aren't
> checked? That mostly passes on adblockpluschrome with just two fixes required
to
> options.js:
> 
> "camelcase": ["error", {"properties": "never"}]

Well spotted. I guess we could use that option in combination with an eslint
comment in the prefs module. It's not ideal though, preferable we'd check
property definitions as well, just not lookups which is kinda stupid that ESLint
does that in the first place (if an API doesn't use camelcase, it's generally
the mistake of the code that defines the API, not of the code that uses it).

On 2017/02/03 09:28:52, kzar wrote:
> OK, I've now checked for duplicates and for any redundant rules which are on /
> off by default. Here's the new review:
> https://codereview.adblockplus.org/29374555/

Did you try to upload a new patch set to this review? The COLLABORATOR line in
the review's description aboce should allow you to so. It's one of Rietveld's
hidden features. I never tried it, and have no idea whether it works with our
Rietveld instance, though.