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

infra(deps): add eslint unicorn #2418

Merged
merged 5 commits into from
Oct 6, 2023
Merged

Conversation

Shinigami92
Copy link
Member

This PR adds https://github.com/sindresorhus/eslint-plugin-unicorn to out eslint configuration

For now this is just the start of using this huge improvement-driven eslint plugin

I will do individual PRs for each rule, so the review process can be optimized and also individual rules can be discussed if needed


there is a PR that is an approximation of what it looks like when all is finished #2417

@Shinigami92 Shinigami92 added c: chore PR that doesn't affect the runtime behavior c: dependencies Pull requests that adds/updates a dependency labels Sep 23, 2023
@Shinigami92 Shinigami92 added this to the vAnytime milestone Sep 23, 2023
@Shinigami92 Shinigami92 self-assigned this Sep 23, 2023
@Shinigami92 Shinigami92 requested a review from a team as a code owner September 23, 2023 21:21
@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #2418 (793ab79) into next (c0386ac) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2418      +/-   ##
==========================================
- Coverage   99.59%   99.59%   -0.01%     
==========================================
  Files        2823     2823              
  Lines      255523   255523              
  Branches     1107     1105       -2     
==========================================
- Hits       254488   254480       -8     
- Misses       1007     1015       +8     
  Partials       28       28              

see 1 file with indirect coverage changes

ST-DDT
ST-DDT previously approved these changes Sep 23, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me, but I'm not sure whether we actually need it.

.eslintrc.js Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from a team September 23, 2023 23:03
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Sep 23, 2023
@Shinigami92
Copy link
Member Author

This PR looks good to me, but I'm not sure whether we actually need it.

IMO we should definitely use eslint-plugin-unicorn, because you already asked for "is there a lint rule for XY" here #2284 (comment)

and secondly: I'm sick of getting asked such question, so I now try to bring forward more linting to prevent such questions proactively

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @ST-DDT here. While this is cool and all, what is the actual value we gain from this? I've seen people go absolutly crazy over static code analysis and I fear that we might run into one of those rabbit holes here as well. If one of those rules is actually helpful I'm here for enabling them, but do we really need all of them? I'll have read into the explaination why we should use each rule but that will take a while.

.eslintrc.js Outdated Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member

[...] I'm sick of getting asked such question, so I now try to bring forward more linting to prevent such questions proactively

I'm sick of the lack of design patterns getting used in this library to ensure code readabilty and maintainablity, but I was getting shut down as it is "kind of overengineered". How is "proactively" disallowing certain programming styles though static code analysis not overengineered tho?

@ST-DDT
Copy link
Member

ST-DDT commented Sep 23, 2023

This PR looks good to me, but I'm not sure whether we actually need it.

IMO we should definitely use eslint-plugin-unicorn, because you already asked for "is there a lint rule for XY" here #2284 (comment)

and secondly: I'm sick of getting asked such question, so I now try to bring forward more linting to prevent such questions proactively

I'm sorry.

@Shinigami92
Copy link
Member Author

This PR looks good to me, but I'm not sure whether we actually need it.

IMO we should definitely use eslint-plugin-unicorn, because you already asked for "is there a lint rule for XY" here #2284 (comment)
and secondly: I'm sick of getting asked such question, so I now try to bring forward more linting to prevent such questions proactively

I'm sorry.

Accepted 🙂

Now I started to put a whole half day already to find a way to ensure a modern, uniform, stable code base, so there is no much place for doing it this way or that way
even some performance improvements are uncovered from that when we go this route long term
remember that eslint is to reduce load of maintainers and put the review step into un-biased non-human automation

it's a bit like the script, generation of locale files and auto-generating the docs

@matthewmayer
Copy link
Contributor

Don't take this the wrong way, but i feel that we do spend a lot of time debating on adding more and more lint rules; and although the goals of a more modern and consistent codebase are noble, this shouldn't come at the expense of actually working on new features and fixing bugs.

@matthewmayer
Copy link
Contributor

Obviously its partly personal preference, but i would much rather be brainstorming on adding a food module (1593) or figuring out steps towards solving bundle shaking (1791) than debating precisely when to use spread syntax :D

@Shinigami92
Copy link
Member Author

Don't take this the wrong way, but i feel that we do spend a lot of time debating on adding more and more lint rules; and although the goals of a more modern and consistent codebase are noble, this shouldn't come at the expense of actually working on new features and fixing bugs.

Good that I'm not the only developer contributing to this project and stuff can be done in parallel

Obviously its partly personal preference, but i would much rather be brainstorming on adding a food module (1593) or figuring out steps towards solving bundle shaking (1791) than debating precisely when to use spread syntax :D

Feel absolutely free to do that, I don't hold you back
But I'm glad we are on the same side: "stop debating precisely when to use spread syntax", just let the linter do its job and use the eslint fixer
No more code review debation, no more personal preference, this is exactly what I'm trying to solve here

It's right now consuming a lot of my time while doing reviews to make up argues why something should be this or that way
Now eslint is telling you and you even have documentation by the individual plugins and you can just jump with only one mouseclick from the hover-tooltip to their docs and learn why something is better over the other
example: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-array-find.md

Array#find() breaks the loop as soon as it finds a match and doesn't create a new array.

just by applying this lint rule, we get not even stylistic choices, but even performance-fixes

@ST-DDT ST-DDT assigned ST-DDT and unassigned Shinigami92 Oct 3, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Oct 3, 2023

Team Decision

ST-DDT will take over this PR and check whether all recommended rules or individual rules are shorter.

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Oct 3, 2023
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Oct 3, 2023
@ST-DDT ST-DDT requested review from xDivisionByZerox and a team October 5, 2023 19:52
@xDivisionByZerox xDivisionByZerox added the c: infra Changes to our infrastructure or project setup label Oct 6, 2023
@xDivisionByZerox xDivisionByZerox changed the title chore(deps): add eslint unicorn infra(deps): add eslint unicorn Oct 6, 2023
@xDivisionByZerox xDivisionByZerox removed the c: chore PR that doesn't affect the runtime behavior label Oct 6, 2023
@xDivisionByZerox xDivisionByZerox merged commit 1ee46a7 into next Oct 6, 2023
24 checks passed
@xDivisionByZerox xDivisionByZerox deleted the add-eslint-unicorn-dep branch October 6, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: dependencies Pull requests that adds/updates a dependency c: infra Changes to our infrastructure or project setup s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants