Audit 2.0 rules for deprecation or inclusion in 3.0 about eslint-config-netflix HOT 9 CLOSED

@klebba — That was a lot of documentation reading! I mostly agree with your sentiment (thanks for writing in a way that was easy to review, by the way!). There are just a handful of things that I may disagree on.

from eslint-config-netflix.

Reconciling our comments so far, here are the items still needing to settle:

array-callback-return

Keep. Agree with your rationale above.

curly

Keep (for now). Will be removed later along with semi regardless.

dot-notation

Remove. This syntax is not prone to inducing bugs. Choice is mostly stylistic. A quick scan of our codebase reveals frequent use of this pattern (IMO that is because it is often situationally appropriate/natural)

no-bitwise

Keep. Could flip a coin on whether or not we should treat typos as a syntax problem to warn about.

no-loop-func

Keep. However if this rule incurs a notable performance cost we should remove it.

no-sequences

Keep. Could flip a coin on whether or not we should treat typos as a syntax problem to warn about.

from eslint-config-netflix.

Sounds good to me @klebba! I agree with your summaries above 👌

from eslint-config-netflix.

Assessment of rules still needing consideration:

array-callback-return

REMOVE: stylistic preference regarding explicit rather vs implicit return statements

camelcase

REMOVE: stylistic preference regarding naming convention / word delimiters

class-methods-use-this

REMOVE: stylistic preference regarding organizing code as static vs class methods

curly

REMOVE: attempts to mitigate bugs by encouraging curly braces around blocks

dot-notation

REMOVE: stylistic preference regarding property accessor syntax

guard-for-in

INCLUDE: perhaps this is a useful reminder as a warning; also would include prefer-object-has-own

new-cap

REMOVE: stylistic preference regarding constructor names

no-bitwise

REMOVE: attempts to mitigate bugs by discouraging valid syntax

no-caller

INCLUDE: warns about formerly common, now deprecated JS feature

no-empty

REMOVE: prone to warning about a non-issue during the authoring process

no-empty-function

REMOVE: prone to warning about a non-issue during the authoring process

no-eval

INCLUDE: the appropriate enforcement is via Content-Security-Policy rules regardless

no-extend-native

INCLUDE: often considered to be an anti-practice

no-iterator

REMOVE: warns about obsolete (and proprietary/ultra obscure) syntax

no-lone-blocks

REMOVE: prone to warning you about a non-issue during the authoring process

no-loop-func

INCLUDE: may offer some bug mitigation value

no-multi-str

REMOVE: archaic concern; this is valid JavaScript now

no-new

REMOVE: rule warns about valid syntax

no-plusplus

REMOVE: rule warns about valid constructor use

no-proto

INCLUDE: warns about formerly common, now deprecated JS feature

no-script-url

INCLUDE: same rationale as no-eval

no-sequences

REMOVE: attempts to mitigate bugs by discouraging valid syntax

no-template-curly-in-string

REMOVE: attempts to mitigate bugs by discouraging valid string content

no-underscore-dangle

REMOVE: the rule description states "As far as naming conventions for identifiers go, dangling underscores may be the most polarizing in JavaScript."

no-unused-expressions

INCLUDE: attempts to mitigate logical errors in application flow

no-useless-constructor

REMOVE: prone to warning you about a non-issue during the authoring process

no-useless-rename

REMOVE: attempts to mitigate a non-issue

prefer-rest-params

REMOVE: rule warns about valid syntax

prefer-spread

REMOVE: rule warns about valid (not discouraged or deprecated) syntax in favor of ES6 shorthand for the same

prefer-template

REMOVE: rule warns about use of string concatenation

strict

REMOVE: no longer relevant in modern JS contexts

from eslint-config-netflix.

Note:

• no-fallthrough
• no-prototype-builtins

These may have been relaxed from error to warning in order to ensure former versions of ESLint would not bomb out in their occurrence (vs continue parsing)

from eslint-config-netflix.

@theengineear I know it's a lot to review but there's no specific urgency here. I'm looking for a cross-check on my disposition of the rules to be dis/included in 3.0 — my hope is to slim this thing down and not leave anyone in the lurch who wants to upgrade

from eslint-config-netflix.

Going to just mark this one-by-one and keep returning to this comment as I go:

🤔 ➖ array-callback-return

• I think I want this still. To me, using these methods carry certain semantics. If you don’t want to actually create an object, different methods exist with the appropriate semantics. I’m mildly (not strongly) opinionated about this.

👍 ➖ camelcase

👍 ➖ class-methods-use-this

👎 ➖ curly

• To me, this is is the kind of thing to pull forward like semi. If I want to be terse, I’d probably use a ternary versus a if, right? I’m mildly (not strongly) opinionated about this.

🤔 ➖ dot-notation

• Sure, this is stylistic, but I also do think it’s a strange access pattern. Only fleetingly opinionated though. I’m fine either way here.

👍 ➕ guard-for-in

👍 ➖ new-cap

🤔 ➖ no-bitwise

• While I agree that it feels somewhat silly to get in the way of a developer legitimately using bitwise operators, it feels actually bad to not warn a developer that accidentally wrote | to revisit to see if they meant ||. I’m tempted to want to keep this one.

👍 ➕ no-caller

👍 ➖ no-empty

👍 ➖ no-empty-function

👍 ➕ no-eval

👍 ➕ no-extend-native

👍 ➖ no-iterator

👍 ➖ no-lone-blocks

👍 ➕ no-loop-func

• This rule is incredibly nuanced. I guess I’m for it, but at some point, I do wonder if we ought to consider the performance cost of rules versus potential value 🤔

👍 ➖ no-multi-str

👍 ➖ no-new

👍 ➖ no-plusplus

👍 ➕ no-proto

👍 ➕ no-script-url

🤔 ➖ no-sequences

• This also feels like a rule that would prevent tricky bugs. I don’t think this is terribly common to do and it would be frustrating to make a mistake and not have it be caught. Again, not super opinionated here, but I think this one might be more helpful than harmful to have.

👍 ➖ no-template-curly-in-string

👍 ➖ no-underscore-dangle

👍 ➕ no-unused-expressions

👍 ➖ no-useless-constructor

👍 ➖ no-useless-rename

👍 ➖ prefer-rest-params

👍 ➖ prefer-spread

👍 ➖ prefer-template

👍 ➖ strict

from eslint-config-netflix.

Thanks for the review; I'll incorporate your feedback / respond soon and ping back here

from eslint-config-netflix.

Addressed by #21

from eslint-config-netflix.