Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cleanup eslint config #454

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

43081j
Copy link
Collaborator

@43081j 43081j commented Mar 13, 2022

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

@fb55
Copy link
Collaborator

fb55 commented Mar 14, 2022

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.

@43081j
Copy link
Collaborator Author

43081j commented Mar 14, 2022

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.

@fb55
Copy link
Collaborator

fb55 commented Mar 14, 2022

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 👀

@43081j
Copy link
Collaborator Author

43081j commented Mar 14, 2022

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

@fb55
Copy link
Collaborator

fb55 commented Mar 16, 2022

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).

@wooorm
Copy link
Collaborator

wooorm commented Mar 16, 2022

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.

@43081j
Copy link
Collaborator Author

43081j commented Mar 16, 2022

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

@43081j
Copy link
Collaborator Author

43081j commented Mar 26, 2022

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.

@fb55
Copy link
Collaborator

fb55 commented Mar 26, 2022

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

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

@43081j
Copy link
Collaborator Author

43081j commented Mar 26, 2022

yup it is. im aware, i understand that.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants