-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
refactor: Move Bazel wrapper commands into separate file #406
Conversation
This in preparation of adding a new "Measure Coverage" command, but I don't want to make the main `extension.ts` file even longer.
bazelTestAllRecursive, | ||
), | ||
vscode.commands.registerCommand("bazel.clean", bazelClean), | ||
...activateCommands(), | ||
vscode.commands.registerCommand("bazel.refreshBazelBuildTargets", () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
tobazel_wrapper_commands.ts
- rename the function
activateCommands
->activateBazelWrapperCommands
- update the commit message accordingly
Option 2:
- also move
copyTargetToClipboard
andrefreshBazelBuildTargets
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
d7acf32
to
f4b84a7
Compare
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.