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

Errors for promise issues in eslint... #8796

Merged
merged 21 commits into from
Aug 14, 2024
Merged

Conversation

burtonator
Copy link
Collaborator

@burtonator burtonator commented Aug 8, 2024

Link to Issue

Closes: #8793

Description of Changes

  • The / eslint-rule set was changes so that it's only core/important errors
  • Added this to CI so more lint runs are executed

We will still have to improve our lint rules but at least hard errors are being fixed.

Also, important note. I REMOVED lint-branch-warnings. We don't need it anymore and there are no advantages since we're running lint-diff. Also, lint-branch-warnings will cause false positives in some situations so it's actually problematic to our setup. Basically, if you touch a file, it's seen as 'changed' so you will start getting errors.

"How We Fixed It"

Test Plan

  • it's already tested due to being in CI

Deployment Plan

Other Considerations

@burtonator burtonator marked this pull request as ready for review August 9, 2024 17:29
Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

  • Not sure we want to remove eslint warning recommendations
  • We should fix the promise issues instead of ignoring them with the comments cause then we won't ever get around to fixing them and assume the comments are correct.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Show resolved Hide resolved
libs/model/src/contest/Contests.projection.ts Show resolved Hide resolved
libs/model/src/middleware/authorization.ts Outdated Show resolved Hide resolved
libs/model/src/middleware/authorization.ts Outdated Show resolved Hide resolved
libs/model/src/models/index.ts Show resolved Hide resolved
libs/model/src/models/index.ts Show resolved Hide resolved
libs/sitemaps/src/createDatabasePaginator.ts Show resolved Hide resolved
@burtonator
Copy link
Collaborator Author

burtonator commented Aug 13, 2024

I wanted to clarify the intent of this PR.

I only want to enable lint-diff and then grandfather anything necessary.

Fixing each issue would mean potentially introducing bugs in this PR plus fixing too many issues at once.

Also, if I fix them, I could fix them wrong.

So every change might increase PR latency. Just grandfathering them (along with probably 30 there ones via the lint-diff config) was the strategy I'm going for.

I'm totally fine if we want to come back and resolve these after the fact but we also have a lot of other promise violations int he code that are grandfathered - we should probably fix those too.

I also put back in the lint-branch-warnings for now until I have to the time to make sure lint-diff works with all our other rules.

Without this change we were potentially adding new thread bugs and since these are often pretty serious errors I wanted to fix this quickly.

@burtonator burtonator merged commit b8e5898 into master Aug 14, 2024
9 checks passed
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.

Enable promise eslint rules across all repositories as our default eslint rules.
4 participants