Support choosing checkout branch method when status is not empty #2494
Conversation
Allow choosing a checkout branch method when status is not empty
extrawurst
requested review from
cruessler
and removed request for
dotslashdan
|
I have looked at this a while ago extensively. Would like a second pair of eyes @cruessler - please give it a go aswell locally, not just by code-review so we do not miss potential regressions. give me a |
src/popups/checkout_option.rs
Outdated
| if self.local { | ||
| checkout_branch( | ||
| &self.repo, | ||
| &self.branch.as_ref().expect("No branch").name, | ||
| )?; | ||
| } else { | ||
| checkout_remote_branch( | ||
| &self.repo, | ||
| self.branch.as_ref().expect("No branch"), | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Would it make sense, perhaps as a follow-up, to extract this code that is very similar to existing code in BranchListPopup::switch_to_selected_branch into a shared location?
There was a problem hiding this comment.
Should I add a checkout method to BranchInfo?
There was a problem hiding this comment.
Lets as much as possible layer this new functionality on top. So we do not have to change as many existing tests.
So lets do a new checkout_with_method in asyncgit that bundles the logic you did now inside our App, method can then be a parameter
There was a problem hiding this comment.
Sorry, I don’t fully understand what you mean. Do you mean I should implement a function called checkout_with_method in asyncgit::BranchInfo, which performs the checkout and then runs some additional effect functions through a callback parameter?
|
@Fatpandac I compared our behavior when choosing |
There was a problem hiding this comment.
Considering the findings I would further argue lets move the logic to checkout with different options to asyncgit and lets cover them with proper unit tests that recreate the various scenarios and the expected results/errors we want to achieve. this is a fundamental enough feature to warrant that quality and resilience for future changes not breaking it
|
@Fatpandac if the stash staging and non-staged changes is gonna be hard (I assume so, I am not aware we have that machinery yet) then lets for now rip out the stashing option and support only |
|
@extrawurst I agree. I'll remove the |
4 participants
This Pull Request fixes/closes #2404.
It changes the following:
I followed the checklist:
make checkwithout errors