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

ci: repair lint setup and run it in CI #15720

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

ci: repair lint setup and run it in CI #15720

wants to merge 6 commits into from

Conversation

DonIsaac
Copy link
Contributor

What does this PR do?

  • Replace eslint with oxlint, which was not configured correctly. Before this PR, bun lint simply crashed.
  • Configure oxlint to only look for a set of strictly incorrect code
  • Add a lint job to CI. While bun lint will display warnings locally, this job will not.
  • Fix error-level lint rule violations found. These are strictly (minor) bug fixes.

How did you verify your code works?

Build + test locally

@DonIsaac DonIsaac requested a review from Electroid December 12, 2024 01:50
@robobun
Copy link

robobun commented Dec 12, 2024

@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Dec 12, 2024

I think we're going to need to be very specific of what files have which linter rules apply. We necessarily do weird things in various tests. Things like ReferenceError should definitely be caught, but most stylistic opinions should be enforced by formatter and not by linter

And I don't think spending the time fixing made-up linter errors is higher priority than improving the Node.js passing test %

@DonIsaac
Copy link
Contributor Author

@Jarred-Sumner I've greatly narrowed the scope of files subject to linting. You'll find the full list in the ignorePatterns property in oxlint.json. I'm also only checking for correctness issues. I've already found a precedence bug in node:stream.

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.

4 participants