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: loosen up eslint rules #7335

Merged
merged 3 commits into from
Nov 29, 2024
Merged

chore: loosen up eslint rules #7335

merged 3 commits into from
Nov 29, 2024

Conversation

FRSgit
Copy link
Contributor

@FRSgit FRSgit commented Nov 27, 2024

@FRSgit FRSgit self-assigned this Nov 27, 2024
@FRSgit FRSgit requested a review from a team as a code owner November 27, 2024 14:28
Copy link

changeset-bot bot commented Nov 27, 2024

⚠️ No Changeset found

Latest commit: d4b279a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@Razz21 Razz21 left a comment

Choose a reason for hiding this comment

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

I was wondering if wouldn't be feasible to provide each config there with a set of strict rules (enabled conditionally) which would enable some of these "blocking" rules.
These rules would be intended for a development, while for demos/ clients projects we would deliver configs with strict rules disabled (or even better - without strict enabled)

@FRSgit
Copy link
Contributor Author

FRSgit commented Nov 27, 2024

@Razz21 that proposal makes sense, I'll add it to our backlog!
Edit: added here: https://alokai.atlassian.net/browse/UST-1848

@FRSgit FRSgit requested a review from Razz21 November 27, 2024 15:56
Copy link
Collaborator

@bartoszherba bartoszherba left a comment

Choose a reason for hiding this comment

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

Why do we change these rules?

Copy link
Collaborator

@bartoszherba bartoszherba left a comment

Choose a reason for hiding this comment

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

Changes like that affects the whole company. It will have a great impact on the code quality and it should be not merged in such task. I understand the problem of reducing warnings when we lint the code but the goal should be do align with the best possible set of rules that will help us maintain the code, not to remove rules to address the threshold requirement. Such changes deserves RFC with an explanation of why do we things like we do. Currently warns are mostly harmless and it is a shame that we ignore them in the first place. If we turn off relative rules it will be nothing more like turning your back to the problem.

engineering-toolkit/eslint-config/src/ecma.js Show resolved Hide resolved
engineering-toolkit/eslint-config/src/typescript.js Outdated Show resolved Hide resolved
engineering-toolkit/eslint-config/src/ecma.js Show resolved Hide resolved
"@typescript-eslint/no-use-before-define": [
"warn",
{
functions: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for making that change. I never understood why we disallowed ordering functions by relevance and making good use of hoisting.

@lukasborawski
Copy link
Collaborator

Can we get use cases and reasoning? The statement that some rules are problematic is not so mature here. We need a wider discussion about these rules, some of them are a real pain in the ass, but at the same time, they're quite important, and someone decided to use them not on top of a hunch, right?

@FRSgit
Copy link
Contributor Author

FRSgit commented Nov 28, 2024

@lukasborawski @lsliwaradioluz @bartoszherba Sorry, I thought those rules will be rather insignificant for you.
If you could - please write here which ones can be merged into this repository in your opinion.

I'll move everything else into storefronts repo as I don't want to waste too much of your time on eslint-rules-related discussions 😄

Sorry again, should've pinged you for your opinion

@Razz21
Copy link
Contributor

Razz21 commented Nov 28, 2024

I think this package may miss @typescript-eslint package as a dependency. I receive errors when trying to override these rules in a repo where this package is not explicitly installed, event though it is a dependency of typescript-eslint already

@FRSgit FRSgit force-pushed the chore/loosen-up-eslint-rules branch 2 times, most recently from 2315801 to 9e8ef46 Compare November 29, 2024 10:00
@FRSgit FRSgit force-pushed the chore/loosen-up-eslint-rules branch from 9e8ef46 to d4b279a Compare November 29, 2024 10:01
@FRSgit FRSgit merged commit c893a44 into main Nov 29, 2024
1 check passed
@FRSgit FRSgit deleted the chore/loosen-up-eslint-rules branch November 29, 2024 10:03
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.

5 participants