-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(stack reorder): Implement PickCmd #126
feat(stack reorder): Implement PickCmd #126
Conversation
Current Aviator status
This PR was merged using Aviator. |
// Use FastForward to avoid always amending commits. | ||
FastForward: true, | ||
}) | ||
if conflict, ok := errutils.As[git.ErrCherryPickConflict](err); ok { |
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 if this is super bad practice. The alternative is doing
var conflict git.ErrCherryPickConflict
if errors.As(err, &conflict) {
which is only one extra line.
Let me know if this burns your eyes to look at and I'll change it 😅.
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.
Really? I feel like I do this assign-in-if frequently to limit the scope. (Also mentioned in https://go.dev/doc/effective_go#if)
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.
Yeah, the only difference here is that we have to declare the concrete type of the error which isn't possible without using generics here. There's no
if var conflict git.ErrCherryPickConflict; errors.As(err, &conflict) { ... }
construct in Go.
I guess I could also do
if conflict := git.ErrCherryPickConflict{}; errors.As(err, &conflict) { ... }
but I've never seen that usage before.
internal/git/cherrypick.go
Outdated
Resume CherryPickResume | ||
} | ||
|
||
type CherryPickResult struct { |
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 guess initially thinking about returning this, but decided not to use this?
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.
Yep, good catch.
// Use FastForward to avoid always amending commits. | ||
FastForward: true, | ||
}) | ||
if conflict, ok := errutils.As[git.ErrCherryPickConflict](err); ok { |
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.
Really? I feel like I do this assign-in-if frequently to limit the scope. (Also mentioned in https://go.dev/doc/effective_go#if)
/aviator stack merge |
This pull request failed to merge: not queued. Remove the |
/aviator stack cancel |
This PR is stacked on top of #123 which was merged. This PR (and its dependents) need to be synchronized on the commit that was merged into # Switch to this branch
git checkout travis/mer-2345-av-stack-reorder-implement-pick
# Sync this branch (and its children) on top of the merge commit
av stack sync
# Optionally update pull requests to match new stack
av stack submit |
2f7d349
to
bb013d7
Compare
Implements the actual
pick
command (along with adding the necessary other things likeRepo.CherryPick
).