-
Notifications
You must be signed in to change notification settings - Fork 409
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
draft: fix: stop showing output tab in unnecessary circumstances #4337
base: develop
Are you sure you want to change the base?
draft: fix: stop showing output tab in unnecessary circumstances #4337
Conversation
Thanks for the contribution @codythomaszeitler - tagging our PM @AnanyaJha on this one to review and see if this is something she'd like us to get in with its current state. We actually will be shipping a new change this week to notifications that you can check out at #4318 (which won't make the extensions less noisy, but more focused at least!). |
d2b82a3
to
b2da3c8
Compare
This issue has been linked to a new work item: W-11853975 |
Hi @codythomaszeitler - thank you for the contribution! @randi274 The overall goal of the request makes sense to me, but there's a few things I'd like to confirm:
[I ran into some errors while trying to build this locally, would be lovely if you could share a video or gif of the final functionality] Although the Problems tab contains info about specific errors, some of our users do rely on the extra info provided by the CLI output. We don't need to auto-switch the tabs for the users, but we should provide them with the same level of info if desired. |
Hey @codythomaszeitler - wanted to check in on this PR! We've got some conflicts with our current state of the repo. Ananya is on board with the changes, as long as we haven't impacted the majority of our command output + the CLI output. Given that some time has passed and you originally noted some hesitation about the usefulness of this PR in your own workflow, would you like to gear up on it again and get it over the finish line with updates + test changes, or opt to close this one for now? |
Hey @randi274 . I am willing to get this over the finish line. I do want to clarify some things though. For my vscode, my output tab does seem to be auto focusing to the output tab when something fails. I am not sure if I am running an old version and that is the reason this is happening, but test failures and deploy failures do seem to focus output for me. (Here is a gif for a deploy failure). (This is in response to @AnanyaJha 's third bullet point above) For my developer experience, I would very much appreciate to have the problems tab auto focus in both the deployment failure and in the test failure case. If not that, just not having the IDE focus the output tab would be preferable. My coworkers use intellij with illuminated cloud, and I believe their IDE's auto focus their problems tab when something fails. It normally flows quite naturally for them, since if something has gone wrong, you want to see a clean panel of information denoting why something failed. To get super specific about how I personally work, it normally goes something like this: Write failing test with a function/class that most likely does not exist. Rinse and repeat. Just having to refocus the problems tab gets disorienting, since vscode is swapping windows constantly at the bottom of my screen. I would love to have a toggle that allow me to tell the extension to either 1) focus on the problems view if a deployment / test failure has occurred or 2) do not focus the output tab on all sfdx commands (which I believe it is doing). The changes in this PR should not affect any information that is currently being presented in the output tab. Let me know if you guys need any other information from me! |
That's a great overview of your process @codythomaszeitler! Would you be interested in starting a discussion to see if others follow a similar process, and there might be some broader improvements we can make to the notification experience, such as adding a setting to either focus only on failures, or turn off all notifications? I'd say for the 3rd bullet point (autofocusing on errors), as long as this PR doesn't change what's currently happening, we wouldn't worry about it for merging. To finish this one out, we'd need you to get the latest changes from develop into your branch, and take a stab at writing some unit tests to be code complete. 👍 |
Hey @randi274 I will start a discussion and also get on those unit tests. |
What does this PR do?
This PR does the following:
Stops showing the output tab in the following scenarios:
Deploy/Retrieve
Running of tests
I chose these commands for the following reason:
They are ran all of the time.
Everything you would want to know about the output of the command can already be found in the "Problems" tab in a human readable format.
The LibraryCommandletExecutor now defaults to not showing the output tab when the command is ran. The implementer must state explicitly that the output tab should show on run. I believe this is fair, since the project should be aiming for a more intuitive/readable format than just a large format of text from the CLI.
What issues does this PR fix or reference?
#3644
Functionality Before
Whenever a test or deploy is ran, it would swap to the output tab in vscode.
Functionality After
Whenever a test or deploy is ran, it now does not swap to the output tab in vscode.
Notes: I still need to ensure that I have added/fixed any tests that are failing because of these changes. I just wanted to make sure that I am the right path before committing to any solution. I cannot tell how much better this would make my developer experience if just these two changes were to go through (deploy and run tests not showing output anymore)