parse5 icon indicating copy to clipboard operation
parse5 copied to clipboard

chore: cleanup eslint config

Open 43081j opened this issue 2 years ago • 10 comments

This is prep work for me turning the non-null-assertion rule on to error.

Summary:

  • eslint:recommended wasn't enabled for *.ts so i have enabled it
  • unicorn wasn't used at all so i got rid of it (and conflicted with a bunch of stuff when it was)
  • disabled the no-unused-vars rule since we get this from the compiler anyway (noUnusedLocals, noUnusedParameters)
  • fixed the newly discovered lint errors

@fb55

43081j avatar Mar 13 '22 19:03 43081j

From my understanding, the overrides section extends the base lints. This is also supported by the test failure in #411, which reported several issues found with the latest version of eslint-plugin-unicorn.

I am very curious where the new errors came from, as most of the lint settings shouldn't have changed.

fb55 avatar Mar 14 '22 11:03 fb55

it does extend the base one, except for the extends array. that gets replaced.

ts files didn't have the unicorn rules or the recommended rules because of that

A glob specific configuration can have an extends setting, but the root property in the extended configs is ignored

we get all we need really from eslint, typescript-eslint and prettier.

43081j avatar Mar 14 '22 12:03 43081j

ts files didn't have the unicorn rules or the recommended rules because of that

I am a bit confused as to why we saw errors in https://github.com/inikulin/parse5/runs/5282607937 👀

fb55 avatar Mar 14 '22 12:03 fb55

same :D

all sorts of conflicts happened when i added eslint's rules. especially since half of unicorn's rules are stylistic... we have prettier for that.

if i get chance i can have a look into it, but tbh we're not losing anything important here. i'd still get this and the other blocked pr merged

edit:

i tried reading through eslint's source to see what it really does but it gets spaghetti-like quickly. so im just trusting the docs are right here and assuming we ran into a bug or some unexpected behaviour. with this, we're at least following what is documented and have what we need

43081j avatar Mar 14 '22 12:03 43081j

I just tried replicating this locally, and adding eslint:recommended to the override doesn't change anything for me. The override extends the prettier preset and therefore disables the curly rule that is specified in the rules section. It seems like most of the changes in this PR are due to the curly rule, without it actually being added to the override.

I'd like to keep eslint-plugin-unicorn, as almost none of the rules are actually covered by prettier. Eg. array.filter((e) => ...)[0] should be replaced with a .find, but prettier will never report this (this is a pattern that was actually a part of the codebase).

fb55 avatar Mar 16 '22 12:03 fb55

I’m also a fan of xo / unicorn and would prefer to keep it. While it can be quite annoying at times, it does a ton of things that are useful for clean code, which eslint:recommended and prettier don‘t catch.

wooorm avatar Mar 16 '22 12:03 wooorm

ill put it back before we merge, will update once i've had chance

43081j avatar Mar 16 '22 17:03 43081j

the docs are worded poorly, i think i misunderstood them. apparently an overridden extends should also merge with the base one.

so im pretty confused as to why curly gets enabled here.

this config is by the docs, exactly as they say it should work.

i also switched unicorn and prettier. prettier is right, we don't care for any stylistic rules from unicorn, we have a formatter for that. the other way around, there are some rules which can conflict.

43081j avatar Mar 26 '22 17:03 43081j

so im pretty confused as to why curly gets enabled here.

curly is disabled in eslint-config-prettier, which is part of the override.

fb55 avatar Mar 26 '22 17:03 fb55

yup it is. im aware, i understand that.

but it isn't disabled with this config. even though prettier is extended

43081j avatar Mar 26 '22 17:03 43081j