Possible improvements [config-base] about code-quality-tools HOT 7 CLOSED

Great suggestions @developer239!

I agree with @robertrossmann that only an invalid/bad/wrong code should fail with a non-zero exit code. Others should be considered as a warning saying something smelly is present.

Labeled as major

• block-scoped-var
  • agree
• no-implicit-globals
  • agree
• no-param-reassign
  • agree
• no-useless-catch
  • agree
• radix
  • agree
  • warn should be enough
• no-multi-assign
  • totally agree
• no-negated-condition
  • agree
  • warn should be enough
• no-plusplus
  • agree
  • with option allowForLoopAfterthoughts: true 🤔
• prefer-destructuring
  • agree
  • warn should be enough
• import/no-default-export
  • disagree - too opinionated I would say
  • or another solution might to move it among style rules
  • I agree that with default exports devs sometimes import and "rename" the imported thingy, which is unfortunate and causes inconsistencies which are tricky especially while refactoring something, but let's be honest, most of us use default exports nowadays.

Others

• no-dupe-else-if
  • agree
  • warn should be enough
• no-import-assign
  • agree
• no-prototype-builtins
  • agree
• no-setter-return
  • 🤷‍♂️
• complexity
  • agree
  • with levels 5-7, as @developer239 suggested
• grouped-accessor-pairs
  • agree
  • should be located among style rules
• no-alert
  • agree
• no-constructor-return
  • agree
• no-div-regex
  • agree
  • should be colocated with optional or style rulesets?
• no-empty-function
  • agree
  • warn should be enough
• no-eq-null
  • agree
  • might be too strict - so optional ruleset?
  • this might already be handled by eqeqeq rule, right?
• no-invalid-this
  • agree
• no-return-await
  • 🤔I remember that there was some discussion on our #dev channel on this topic
• no-void
  • agree
• prefer-regex-literals
  • agree
  • warn should be enough
• init-declarations
  • agree
  • optional ruleset
• capitalized-comments
  • agree
  • style ruleset
• max-lines
  • agree if configured properly
  • warn should be enough
  • optional or style rulesets?
• max-lines-per-function
  • agree if configured properly
  • warn should be enough
  • optional or style rulesets?
• no-bitwise
  • agree
• wrap-regex
  • 🤷‍♂️
  • style ruleset?
• no-useless-rename
  • agree
  • optional ruleset?

Missing plugins

• eslint-plugin-absolute-import
  • Wouldn't proper path aliases be a better solution?
  • In my opinion, this is way too dependent on project setup
  • Also, the repo has 0 stars so it is not yet used too much, I would honestly wait with integrating this into our toolbelt
• eslint-plugin-unused-imports
  • If I understand, correctly the purpose of this plugin is the auto-fixer feature for unused imports? If not I don't see any added value as no-unused-vars and no-unused-imports does the thing, right?

cc @dannytce

from code-quality-tools.

@robertrossmann Honestly, I don't mind I just wanted to share my opinion here. 🙂

How I can help right now is I can find rules that we are missing and add them to the config. I created a tool that compares my eslint config with STRV config programmatically. I didn't check all rules myself but I am 99 % sure that rules that I shared here are actually missing in STRV config.

https://github.com/developer239/compare-linters

from code-quality-tools.

Thanks @xhudec for taking your time to review this. 🙂

I pretty much agree with everything you said. Maybe we can keep radix as error because it is easily fixable and the codebase will be more consistent.

About the plugins though:

• eslint-plugin-absolute-import
  • Now it has 1 star 😄 the purpose of the plugin is to warn or error whenever we use relative import in the project. It is not project dependent. It is not auto-fixable but the effort is worth it.

• eslint-plugin-unused-imports
  • Honestly, I don't remember why I started using this thing. 🤔 It can remove unused import automatically. no-unused-imports rule does not exist. But you are right that no-unused-vars warns about the same issue - although it is not auto-fixable. Maybe we can skip this plugin and add it later when we run into some issues in the future. Unused imports shouldn't be in the codebase. 🙂

from code-quality-tools.

ONE MORE IMPROVEMENT 🤤

• it is auto-fixable

'import/order': [
  'error',
  {
    'groups': ['builtin', 'external', 'internal', 'parent', 'sibling', 'index'],
    'alphabetize': {
      'order': 'asc',
      'caseInsensitive': true,
    },
  },
],

from code-quality-tools.

@developer239 Thanks for all these suggestions! I will look through them in the coming days.

Right now I can tell you this much:

Stylistic rules and rules where we cannot with a high certainty say that the code is invalid/bad/wrong in any way shall never cause ESLint to fail with non-zero exit code. This was by design from day 1. It's the only reason why we have semi rule set to warn. 🤷‍♂️ 😇 I know it may seem counter-intuitive, but there are many situations where you really want to ie. use template curlies in a plain string (Sendgrid templates) or you want to make a function explicitly an async function because that's how you design the public API, even though there is no asynchronous work being done in it yet.

If you really want to be the evil dictator, you can always run ESLint with --max-warnings 0 flag on a CI server -> any warning will fail the CI.

And I feel that some other rules mentioned in your Missing rules list are enabled in the ruleset somewhere; are you sure they are missing? 🤔 I will take a look. Some of them look like we should have them.

from code-quality-tools.

Thanks for all the feedback! ❤️ I went through the first section of the rules (Missing rules) and added some here: 471568f. For the ones I did not add, I list my reasoning below. I will try to go through the rest sometime soon. 💪

• no-prototype-builtins: too restrictive
• block-scoped-var: var declarations are already forbidden via rule
• complexity: too restrictive
• no-alert: too restrictive, alert or prompt are quite generic function names and having them flagged in Node.js for no reason is not desireable
• no-constructor-return: Haha, no. 😂 This one's fun - you can return something from a class constructor and whatever you return will be the result of calling new. You can completely bend the returned thing. Very fun, indeed.
• no-div-regex: the thing it tries to "un-confuse" is not confusing at all
• no-empty-function: too restrictive
• no-eq-null: we already require strict comparisons to be used
• no-implicit-globals: Node.js and I believe most modern frontend bundlers already wrap the code in an IIFE so this is redundant and quite restrictive
• no-invalid-this: I tried personally, very restrictive. It's enabled for TypeScript, though 💡
• no-param-reassign: I tried personally, very restrictive
• no-return-await: you should use return await constructs. The performance hazards mentioned in rule docs have been optimised away in recent V8 releases, plus, if you do this, V8 can properly construct an async stack trace when errors occur, substantially improving developer UX (or is it DX, then? 🤔). See https://v8.dev/blog/fast-async and https://v8.dev/docs/stack-trace-api
• no-void: void is awesome, it helps you avoid semicolons 😇 and allows several other tricks.
• init-declarations: too restrictive
• capitalized-comments: conflicts with eslint-disable-* and other @ts-* comment directives 🤷‍♂️
• max-lines & max-lines-per-function: I tried, too restrictive
• no-bitwise: why? They are very useful.
• no-negated-condition: too restrictive. Sometimes you want to take the shorter, falsey path first, or it is simply easier to read when the cond is negated
• no-plusplus: just learn how ASI works 🤷‍♂️ (I can explain if you are interested, I am a huge ASI fan, as you surely know 😇)
• wrap-regex: ...but why. It looks very clear to me even without the wrap.
• prefer-destructuring: I tried, too restrictive. Sometimes it resulted in code that was worse/more difficult to read.
• import/no-default-export: yeaaaah I'm with you on this one, but no. 😇 It's too restrictive for quite a large population of developers, sadly.

from code-quality-tools.

I checked out the other rules where you mention you would increase the severity to error.

For almost all rules there is a possibility that the rule would flag some valid piece of code which would cause a potentially working code to be rejected by CI. This is highly undesireable. I will mention a few rules below where I believe some explanation might be helpful as to why it should not be used/set to error.

• require-await: Declaring a function as async changes the runtime semantics of that function (ie. it will never throw, it can only ever return a rejected promise). In some situations it is beneficial to declare a function as async even though it could be sync today, if you know that there is a high chance you will need to do async work inside it in the future. This way you can future-proof your public API if you work on a library or some other reusable component.
• no-template-curly-in-string: Some libraries, sadly, use ${} syntax as a templating language. Setting this rule to error would flag potentially working and valid code as error.

Most of the other rules are cosmetic/stylistic preferences and therefore should be defined as warnings.

Fun fact: in the past, the original ruleset used to have all those various max-lines-per-function, complexity and other similar rules enabled. You should have seen the haters coming at me from all corners of the office. 🔥 And they were right. The intention might be good (to write easy-to-follow code with not too many branches) but in practice these rules could not get you there; proper code reviews are what can help us write easy-to-follow code. 🕵️

Missing plugins:

As for eslint-plugin-unused-imports, I see no added value here. Most of the use cases can be attained with no-unused-vars and similar.
And with eslint-plugin-absolute-import you would kill a very powerful feature of Node.js so that's a no go. There are situations where you really want to use a relative file path instead of an absolute one, ie. when you create a piece of functionality which should behave like a single unit but actually consists of many parts, and you want to keep those many parts hidden from the rest of the codebase. Those relative imports would always have to be moved together, as a single unit, so there is no point in having an absolute import path for them.

Anyway, thank you so much for taking the time to review the ruleset! I incorporated a lot of your proposed changes, even if with a lower severity level. 😇 ❤️

from code-quality-tools.