refactor: Simpler and more type-safe BazelTaskInfo
#365
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add the `bazelTaskInfo` member to the `BazelTaskDefinition` type so it can be accessed in a more typesafe manner. * Merges `bazel_task_info.ts` into `tasks.ts`. Otherwise, adding the `bazelTaskInfo` member would have introduced a cyclical dependency. Also, the `bazel_task_info.ts` was rather small anyway, so keeping this in a separate file didn't provide much value anyway. Refactoring in preparation for bazel-contrib#362
vogelsgesang
requested review from
coeuvre,
jfirebaugh and
cameron-martin
as code owners
cameron-martin
approved these changes
Apr 4, 2024
vogelsgesang
added a commit
to vogelsgesang/vscode-bazel
that referenced
this pull request
Apr 7, 2024
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.
vogelsgesang
added a commit
to vogelsgesang/vscode-bazel
that referenced
this pull request
Apr 7, 2024
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
pushed a commit
that referenced
this pull request
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 #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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
bazelTaskInfomember to theBazelTaskDefinitiontype so it can be accessed in a more typesafe manner.bazel_task_info.tsintotasks.ts. Otherwise, adding thebazelTaskInfomember would have introduced a cyclical dependency. Also, thebazel_task_info.tswas rather small anyway, so keeping this in a separate file didn't provide much value anyway.Refactoring in preparation for #362