fix(workspace-tree): honor bazel.commandLine.queryExpression for root targets #481
Conversation
… targets Respect the user's bazel.commandLine.queryExpression when querying workspace root targets by replacing ...:* with :all if a root BUILD file exists. Prevents explorer view bloat from generated/shadow root targets when using custom query expressions (e.g., with aspect_rules_js). Only queries root targets if the root BUILD or BUILD.bazel file is present. Closes bazel-contrib#480.
MichaelSbarraKodiak
requested review from
cameron-martin,
coeuvre,
jfirebaugh and
vogelsgesang
as code owners
cbandera
left a comment
cbandera
left a comment
There was a problem hiding this comment.
Apart from the two findings in the code, I am not sure whether the suggested code change will actually do what you hope for.
The code in the modified function getDirectoryItems() will first collect all packages that it finds with the user specified queryExpression. And will initialize the target tree from this in buildPackageTree. After that, only the targets in the root package are queried and added to the tree.
This can be thought of as a lazy initialization of the tree. I.e. query all package names to quickly build a directory tree, but only fill in the details for those nodes of the tree that are expanded. (If you extend a node, the getChildren method will always query the targets for just this package).
The code that you have modified does only query the toplevel targets during initial initialization. And with your change, it will now not only query the toplevel targets but potentially search for targets recursively within the specified scope. This is not correct here.
Having said that, I do have to admit however that the logic is already broken in case a user has specified a custom queryExpression that only queries a subsection of a repo, because in this case, the current logic would nevertheless query and display the toplevel targets anyway. (But maybe that's not too bad...)
| }); | ||
| let targets: BazelTargetTreeItem[] = []; | ||
| // Only get root targets if the root package is present | ||
| const rootBuildFile = |
There was a problem hiding this comment.
How about reusing the getBazelPackageFile() method from bazel_utils.ts?
|
|
||
| if (rootBuildFile) { | ||
| // Replace ...:* with :all in queryExpression | ||
| const queryExpressionRoot = queryExpression.replace(/\.{3}:\*/g, ':all'); |
There was a problem hiding this comment.
I am not sure whether I am missing something. Or maybe this changeset got corrupted by recent refactorings on main. But as far as I understand the code, there is no variable queryExpression here. Maybe you need to rebase onto latest master?
2 participants
bazel.commandLine.queryExpressionwhen querying workspace root targets by replacing...:*with:allif a workspace root BUILD file exists.BUILDorBUILD.bazelfile is present.Closes #480.