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: Fix eslint integration #373

Merged

Conversation

vogelsgesang
Copy link
Collaborator

@vogelsgesang vogelsgesang commented Apr 7, 2024

In #363, I accidentally disabled all eslint checks: I used eslint and thought this would actually run the linter on the complete source tree. Starting from ESLint 9.0, this will actually work (See https://eslint.org/blog/2024/04/eslint-v9.0.0-released/#running-eslint-with-no-file-arguments). But for ESLint 8.x, we still need to use eslint .

This commit fixes that oversight in package.json and rectifies all the fall-out from this mistake.

First, I finished the migration to the new eslint.config.js format. To avoid further bugs, I first enabled TypeScript's type checking via @ts-check. I then followed the migration guide from https://eslint.org/docs/latest/use/configure/migration-guide to fix the config in general.

Doing so, I changed a the linter config in the following way:

  • I disabled the max-classes-per-file linter rule. My change refactor: Simpler and more type-safe BazelTaskInfo #365 violated this rule. But I don't think that this rule provides much value in general. Having one class per file is rather a Java convention than a JavaScript convention...
  • I replaced prefer-arrow/prefer-arrow-functions by the built-in prefer-arrow-callback. Afaict, those checks do the same. This allowed me to remove the dependency on the package eslint-plugin-prefer-arrow.
  • The linting actually alos applied to the eslint.config.js file itself. But not all packages have types, zet. I hence disabled a couple of rules for this file.

With that out-of-the way, I had to fix the actual lint warnings:

  • In `eslint-plugin-prefer-arrow bazel_workspace_info.ts and buildifier.ts: "Unexpected lexical declaration in case block"
  • extension.ts had an (intentionally) dangling promise
  • debug-adapter/connection.ts had an intentional while(true) loop.

In bazel-contrib#363, I accidentally disabled all eslint checks: I used `eslint`
and thought this would actually run the linter on the complete source
tree. Starting from ESLint 9.0, this will actually work (See
https://eslint.org/blog/2024/04/eslint-v9.0.0-released/#running-eslint-with-no-file-arguments)
But for ESLint 8.x, we still need to use `eslint .`

This commit fixes that oversight in `package.json` and rectifies all the
fall-out from this mistake.

First, I finished the migration to the new eslint.config.js format. To
avoid further bugs, I first enabled TypeScript's type checking via
`@ts-check`. I then followed the migration guide from
https://eslint.org/docs/latest/use/configure/migration-guide
to fix the config in general.

Doing so, I changed a the linter config in the following way:
* I disabled the `max-classes-per-file` linter rule. My change bazel-contrib#365
  violated this rule. But I don't think that this rule provides much
  value in general. Having one class per file is rather a Java
  convention than a JavaScript convention...
* I replaced `prefer-arrow/prefer-arrow-functions` by the built-in
  `prefer-arrow-callback`. Afaict, those checks do the same. This
  allowed me to remove the dependency on the package
  `eslint-plugin-prefer-arrow`.
* The linting actually alos applied to the `eslint.config.js` file
  itself. But  not all packages have types, zet. I hence disabled a
  couple of rules for this file.

With that out-of-the way, I had to fix the actual lint warnings:
* In `eslint-plugin-prefer-arrow bazel_workspace_info.ts and
  buildifier.ts: "Unexpected lexical declaration in case block"
* extension.ts had an (intentionally) dangling promise
* debug-adapter/connection.ts had an intentional `while(true)`
  loop.
@cameron-martin cameron-martin merged commit 43195c9 into bazel-contrib:master Apr 7, 2024
4 checks passed
@vogelsgesang vogelsgesang deleted the avogelsgesang-fix-eslint branch April 7, 2024 16:45
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.

2 participants