feat(codelens): Show picker for multiple targets #477
Conversation
bkolb
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.
Thanks for this contribution! I have tested it locally and it works fine. I like this new interactive approach much more then the previous "workaround" with sorting.
Nevertheless, I suggest to improve on the implementation further before merging this to master. See the following comments
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
4a0ab35 to
b55a3fb
Compare
|
Thank you for your feedback. I will work on your comments and post a new version soon. The one I just pushed has an issue that I need to fix. |
bkolb
force-pushed
the
feature/multilineCodeLense
branch
2 times, most recently
from
1b63371 to
b0a63d4
Compare
|
Can I get another round of review, please? |
cbandera
left a comment
cbandera
left a comment
There was a problem hiding this comment.
Thanks for adressing my last set of review comments. The functionality is still given and works fine locally.
Here is a next set of (opinionated) findings with the goal of improving code quality.
(Note that I am not a maintainer, so feel free to take it as a suggestion or ignore as long as none of the maintainers performs a review 😉 )
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
b0a63d4 to
77f8dbd
Compare
|
OK. I pushed again. Let me know what you think. |
|
For the record: I like the latest version a lot and would also be interested in the thoughts of @cameron-martin 😃 |
|
Hi @bkolb do you mind updating your branch with the latest changes from upstream to resolve the merge conflicts? Thanks in advance! |
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
77f8dbd to
faf3594
Compare
|
done |
|
@cameron-martin PTAL! |
|
@cbandera What's left for us to drag this across the finish line? |
|
I didn't test it again after the last merge with master. (And am on mobile only right now) |
|
Tested again locally and it still works fine. I'd be happy to see this in the next release :) |
cameron-martin
changed the title
| // If the command adapter was unspecified, it means this command is being | ||
| // invoked via the command palatte. Provide quickpick build targets for | ||
| // the user to choose from. | ||
| const quickPick = await showDynamicQuickPick({ |
There was a problem hiding this comment.
Not using showDynamicQuickPick any more here looks like a regression. Possibly an issue with merging with #465?
There was a problem hiding this comment.
Good catch, I should have checked for that. The invocation has moved to L55 but is not using the new showDynamicQuickPick. Requires fixing.
| await bazelTestTarget(quickPick); | ||
| } | ||
| return; | ||
| const selectedAdapter = await selectSingleTarget( |
There was a problem hiding this comment.
Again here possibly the same regression.
There was a problem hiding this comment.
Yes, but requires fixing in the same location: L55
|
Ok. I can look at it. How can I test that it works? Is there a test for it? |
|
There are unittests for it, but none at integration level. You can test it manually by running the "Bazel: Build Target" command from the command palette. The dynamic quick pick is a future intended for large workspaces, where waiting for the inital query that loads the results of For testing you can have a look at the debug panel while starting the extension via the launch configuration and interactively trying out the described feature. You should see that multiple queries are exectued instead of just the initial one. |
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
faf3594 to
ce9ce2f
Compare
|
Done. PTAL |
Please see the failing CI run. |
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
ce9ce2f to
f903884
Compare
|
I run the formatter. |
This change modifies the BazelBuildCodeLensProvider to create a target picker in case there is more than one target per category (Copy/Build/Run/Test). The motivation is to provide a better and more user-friendly experience when working with macros in BUILD files. When a macro generates multiple targets, you could run into situations where not all actions were visible anymore.
bkolb
force-pushed
the
feature/multilineCodeLense
branch
from
f903884 to
06659f0
Compare
|
Thanks! |
4 participants
This change modifies the BazelBuildCodeLensProvider to create a target picker in case there is more than one target per category (Copy/Build/Run/Test).
The motivation is to provide a better and more user-friendly experience when working with macros in BUILD files. When a macro generates multiple targets, you could run into situations where not all actions were visible anymore.