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

refactor: Move Bazel wrapper commands into separate file #406

Conversation

vogelsgesang
Copy link
Collaborator

@vogelsgesang vogelsgesang commented Jul 3, 2024

This in preparation of adding a new "Measure Coverage" command, but I don't want to make the main extension.ts file even longer. Hence, I first move all the commands which simply wrap underlying Bazel commands into a separate file.

This in preparation of adding a new "Measure Coverage" command, but I
don't want to make the main `extension.ts` file even longer.
@vogelsgesang vogelsgesang changed the title refactor: Move commands into separate file refactor: Move user-facing commands into separate file Jul 3, 2024
bazelTestAllRecursive,
),
vscode.commands.registerCommand("bazel.clean", bazelClean),
...activateCommands(),
vscode.commands.registerCommand("bazel.refreshBazelBuildTargets", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come some of the commands are still in extension.ts?

Copy link
Collaborator Author

@vogelsgesang vogelsgesang Jul 20, 2024

Choose a reason for hiding this comment

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

My underlying thinking was:

The commands which I moved out into an own file are direct Bazel wrapper commands.
The refreshBazelBuildTargets and copyTargetToClipboard are not wrappers around Bazel commands, but are UI-specific commands.

I obviously failed to communicate this, both in the commit message, the file name and the function name.

I see two options:

Option 1:

  • rename the file commands.ts to bazel_wrapper_commands.ts
  • rename the function activateCommands -> activateBazelWrapperCommands
  • update the commit message accordingly

Option 2:

  • also move copyTargetToClipboard and refreshBazelBuildTargets over

I slightly tend to option 1, because I think we should structure the code based on "features" (wrapping the Bazel commands; providing a tree view; providing code lenses; ...) and not "implementation vehicle" (commands, treeviews, ...). Do you agree with that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense - I agree that organising the code around features is better, so I'm happy with option 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I finally updated this Pull Request 🎉 (took me longer than I hoped for. Too many other things going on...)

@vogelsgesang vogelsgesang changed the title refactor: Move user-facing commands into separate file refactor: Move Bazel wrapper commands into separate file Aug 10, 2024
@vogelsgesang vogelsgesang force-pushed the avogelsgesang-refactor-commands branch from d7acf32 to f4b84a7 Compare August 13, 2024 20:25
@cameron-martin cameron-martin enabled auto-merge (squash) August 13, 2024 21:18
@cameron-martin cameron-martin merged commit b4a8b0c into bazel-contrib:master Aug 13, 2024
2 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.

2 participants