-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
3528 Change branch
terminal-run commands into normal commands w/ proper error handling
#3597
base: main
Are you sure you want to change the base?
3528 Change branch
terminal-run commands into normal commands w/ proper error handling
#3597
Conversation
d2ada6c
to
275d1f4
Compare
@sergiolms Please apply similar changes from #3596 to this PR |
d3f465e
to
fb0d9ac
Compare
fb0d9ac
to
31db995
Compare
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.
Since I don't have too much context going into this, could you help me test it by:
- Describing how to initiate the commands being changed (all the different ways we can get into the delete branch flow that you are aware of)
- Describing what happens before this PR and what happens after the PR changes (essentially, what behavior should I expect to be different when using this PR)
- Describing what areas may have regressions as a result of the PR changes that I should spot-check, if any
I did a quick code-scan and it looks good, just had one nitpick below:
src/commands/git/branch.ts
Outdated
} catch (ex) { | ||
Logger.error(ex); | ||
// TODO likely need some better error handling here | ||
return showGenericErrorMessage( |
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.
We should be handling resumable/retriable errors. For example, if not merged, we should offer to retry with force.
src/commands/git/branch.ts
Outdated
Logger.error(ex); | ||
// TODO likely need some better error handling here | ||
return showGenericErrorMessage( | ||
new BranchError(ex.reason, ex, state.references.map(r => r.name).join(', ')).message, |
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.
Why are we creating a BranchError here? Isn't the one being thrown a BranchError?
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.
Indeed, I don't really like this solution either. When we return the BranchError at git level we don't know the branch names, we just have an array of args. We could either guess the branch names, or change the number of args of git.branch
to be more specific so we can have easier access. In this case we're reusing the BranchError but adding the branch name for context.
Another option is to turn BranchError into a Builder pattern that we can always call like showGenericErrorMessage(ex.WithBranch(branchName))
or something along those lines to provide context on top layers.
src/commands/git/branch.ts
Outdated
@@ -627,7 +637,7 @@ export class BranchGitCommand extends QuickCommand { | |||
} catch (ex) { | |||
Logger.error(ex); | |||
// TODO likely need some better error handling here | |||
return showGenericErrorMessage('Unable to rename branch'); | |||
return showGenericErrorMessage(new BranchError(ex.reason, ex, state.name).message); |
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.
Same here
66762d4
to
58551c8
Compare
4794ca0
to
d093bc3
Compare
d093bc3
to
e9baa26
Compare
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.
Several build errors here, as well as conflicts with main. Will test once it's rebased and the errors are resolved
128b31b
to
5ed1cf7
Compare
5ed1cf7
to
bed464f
Compare
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.
Not sure what commands are affected, and the only one I noticed was branch delete, so I tried it with a few cases and looks like it works and doesn't open my terminal.
LGTM - let me know if there are other cases I should test
bed464f
to
4f145da
Compare
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.
When attempting to delete a remote branch that no longer exists, I do not get the right error message in the notification area. I get a push error that doesn't describe the issue well:
It is true that the operation happening under the hood is git push -d origin cool-branch
but I expected an error relating to "unable to delete branch because the remote branch was already deleted" or however you have the messaging set up.
The error I get in the logs is more descriptive/accurate:
4f145da
to
ead29af
Compare
True. Since this action is running under git push it wasn't covered by the git branch errors. I added an additional case to git push to handle that case: |
ead29af
to
965edeb
Compare
965edeb
to
ab4f84c
Compare
# Conflicts: # src/env/node/git/localGitProvider.ts
# Conflicts: # src/env/node/git/git.ts
# Conflicts: # src/config.ts
# Conflicts: # src/env/node/git/localGitProvider.ts
ab4f84c
to
f502b49
Compare
Description
Part 2 including changes to
branch delete
.Resolves #3528
Example of error:
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses