.eslintrc.json - Issue 29346539: Issue 3692 - Created global eslintrc

Side by Side Diff: .eslintrc.json

Issue 29346539: Issue 3692 - Created global eslintrc (Closed)

Patch Set: Extended Palant's starter based on https://adblockplus.org/coding-style Created June 25, 2016, 6:34 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 {

	2 "extends": "eslint:recommended",

	3 "rules": {

	4 /* MDN General practices */
	kzar 2016/09/16 10:33:51 (It surprised me to see comments in a JSON file bu (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 ) juliandoucette 2016/10/13 14:14:08 Acknowledged. Let me know if you think we should Show quoted text 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.
	5 "no-else-return": "warn",

	6 "no-console": "warn",

	7 "no-debugger": "warn",

	8 "no-alert": "warn",
	kzar 2016/09/16 10:33:50 There are some valid uses of alert in adblockplusc 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? Wladimir Palant 2016/09/16 11:37:06 Actually, using console.error() can be valid when Show quoted text 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). kzar 2016/09/16 12:12:57 Hmm good point. I guess I'm easy between options 2 Show quoted text 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. juliandoucette 2016/10/13 14:14:10 Acknowledged. I changed this to rely on eslint:re Show quoted text 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.
	9 "linebreak-style": ["warn", "unix"],
	kzar 2016/09/16 10:33:50 Shouldn't linebreak-style be an error too, or am I Shouldn't linebreak-style be an error too, or am I missing a valid use case? juliandoucette 2016/10/13 14:14:09 Done. Show quoted text 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.
	10 "indent": ["warn", 2],

	11 "valid-jsdoc": "warn",

	12 "eol-last": "warn",
	Wladimir Palant 2016/09/16 08:07:42 This is already part of eslint:recommended, we don 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. juliandoucette 2016/10/13 14:14:08 Done. Show quoted text 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.
	13 "vars-on-top": "off",
	Wladimir Palant 2016/09/16 08:07:43 This isn't part of eslint:recommended, no point sw This isn't part of eslint:recommended, no point switching it off. juliandoucette 2016/10/13 14:14:08 Done. Show quoted text On 2016/09/16 08:07:43, Wladimir Palant wrote: > This isn't part of eslint:recommended, no point switching it off. Done.
	14 /* MDN Naming and formatting code */

	15 "no-irregular-whitespace": "warn",
	Wladimir Palant 2016/09/16 08:07:43 Like above, this is already part of eslint:recomme Like above, this is already part of eslint:recommended. kzar 2016/09/16 10:33:51 Yea and it should really be an error. One time a z Show quoted text 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 juliandoucette 2016/10/13 14:14:10 Done. Show quoted text On 2016/09/16 08:07:43, Wladimir Palant wrote: > Like above, this is already part of eslint:recommended. Done.
	16 "no-trailing-spaces": "warn",

	17 "max-len": ["warn", 80],

	18 "brace-style": ["warn", "allman"],

	19 /* MDN JavaScript practices */

	20 "no-eq-null": "off",
	Wladimir Palant 2016/09/16 08:07:42 This isn't part of eslint:recommended, no point sw This isn't part of eslint:recommended, no point switching it off. juliandoucette 2016/10/13 14:14:09 Done. Show quoted text On 2016/09/16 08:07:42, Wladimir Palant wrote: > This isn't part of eslint:recommended, no point switching it off. Done.
	21 "no-redeclare": "error",
	Wladimir Palant 2016/09/16 08:07:43 This is already part of eslint:recommended. This is already part of eslint:recommended. kzar 2016/09/16 10:33:51 To play devil's advocate perhaps it's good to spec Show quoted text 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.) Wladimir Palant 2016/09/16 11:37:06 I think that eslint:recommended usually changes wh Show quoted text 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. kzar 2016/09/16 12:12:57 Fair enough. Show quoted text 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. juliandoucette 2016/10/13 14:14:08 Done. Show quoted text 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.
	22 "consistent-return": "error",
	kzar 2016/09/16 10:33:51 Does everyone agree with consistent-return? To me 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. Wladimir Palant 2016/09/16 11:37:06 Yes, consistent-return is important and actually s Show quoted text 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. juliandoucette 2016/10/13 14:14:08 Done. Show quoted text 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. kzar 2017/01/31 10:17:48 One example where I think consistent-return might Show quoted text 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
	23 "no-undef": "error",
	Wladimir Palant 2016/09/16 08:07:43 This is already part of eslint:recommended. This is already part of eslint:recommended. juliandoucette 2016/10/13 14:14:09 Done. Show quoted text On 2016/09/16 08:07:43, Wladimir Palant wrote: > This is already part of eslint:recommended. Done.
	24 "no-unused-vars": "warn",
	Wladimir Palant 2016/09/16 08:07:43 The reason I switched this rule off: it produces t 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 Agreed, it's better to not use a parameter than si Show quoted text 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. juliandoucette 2016/10/13 14:14:09 Done. Show quoted text 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.
	25 "no-array-constructor": "warn",

	26 "no-new-object": "warn",

	27 /* ABP General */

	28 "no-warning-comments": "warn",

	29 /* Palant recommends */
	kzar 2016/09/16 10:33:50 Nit: Mind adding a newline before each new section Nit: Mind adding a newline before each new section? juliandoucette 2016/10/13 14:14:09 Done. Show quoted text On 2016/09/16 10:33:50, kzar wrote: > Nit: Mind adding a newline before each new section? Done.
	30 "array-bracket-spacing": "warn",

	31 "arrow-spacing": "warn",

	32 "comma-spacing": "warn",

	33 "comma-style": "warn",

	34 "computed-property-spacing": "warn",

	35 "key-spacing": "warn",

	36 "keyword-spacing": "warn",

	37 "new-parens": "warn",

	38 "no-spaced-func": "warn",

	39 "no-var": "warn",
	kzar 2016/09/16 10:33:51 We need to use var in adblockpluschrome in some pl We need to use var in adblockpluschrome in some places, for scripts that aren't transpiled like content scripts. Wladimir Palant 2016/09/16 11:37:06 This is an ES6 config, and we are already discussi Show quoted text 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.
	40 "no-whitespace-before-property": "warn",

	41 "object-curly-spacing": "warn",

	42 "operator-assignment": "warn",

	43 "operator-linebreak": "warn",

	44 "padded-blocks": ["warn", "never"],

	45 "prefer-rest-params": "warn",

	46 "prefer-spread": "warn",
	kzar 2016/09/16 10:33:51 I don't mind using the spread operator instead of 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 We definitely could and should. Note that we are Show quoted text 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:57 Perhaps I'll give it a go if I ever find time. I v Show quoted text 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/10/13 14:14:09 Acknowledged. Show quoted text 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.
	47 "require-yield": "warn",

	48 "quotes": ["warn", "double", {"avoidEscape": true}],

	49 "semi": "warn",

	50 "semi-spacing": "warn",

	51 "space-before-function-paren": ["warn", "never"],

	52 "space-in-parens": "warn",

	53 "space-infix-ops": "warn",

	54 "space-unary-ops": "warn",

	55 "spaced-comment": "warn",

	56 "yield-star-spacing": "warn"

	57 }

	58 }

OLD	NEW

« no previous file with comments | « no previous file | no next file » | no next file with comments »