Add Squash merge #3566
Add Squash merge #3566
Conversation
fix typo fix checkout branch
add langage and menu pannel
|
Hello, I finished the feature, and it seems to work well, I will add tests in futures days, but I would like your feedback already if you feel it's needed. The only problem is to generate the cheatsheet, they seem to be empty for the squash, but I don't find where the generate takes them from. It's my first PR, hope it's good |
| if checkedOutBranchName == refName { | ||
| return func() error { return errors.New(self.c.Tr.CantMergeBranchIntoItself) } | ||
| } |
There was a problem hiding this comment.
What Stefan suggested in the original PR was to use the DisableReason field exposed by the MenuItem type to inhibit the actions instead of throwing an error when they're invoked.
Do to this, you should move this check to the upper level (the SquashBranch function) and conditionally set the DisabledReason field in case checkedOutBranchName == refName. Also, consider that both those menu options are disabled for the same reason.
|
I have your suggestion which seems indeed better suited but there's a bug and I can't figure out if it's mine or not. The feature is always disabled even if the ShowErrorInPanel is set to false. Do you have any idea why ? |
There was a problem hiding this comment.
@noahfraiture - The ShowErrorInPanel function just changes how lazygit displays an error when the user tries to run a disabled action. The DisabledReason field must be nil to make an action available, so you can conditionally assign your structure to it when the required conditions for that command are met.
That said, I'd suggest updating GetDisabledReason to disable the menu in the case of merging the checked-out branch, instead of disabling the individual menu options. This approach is similar to how we handle rebase operations. Here is an example of what I would do: c36bbb1.
Also, I think it would be better to clean up the history of this branch before proceeding to merge it.
pkg/i18n/english.go
Outdated
| SquashInFiles: "Are you sure you want to squash '{{.selectedBranch}}' into current working tree?", | ||
| SquashInCommit: "Are you sure you want to squash '{{.selectedBranch}}' into '{{.checkedOutBranch}}' in a new commit?", |
There was a problem hiding this comment.
These should be descriptions instead of questions. We don't need the user to confirm their action in this case. Can we change them?
fix: type and rename files add refresh remove current branch in squashFiles tooltip
add squash in tests
add squash without commit
|
Thank you for your feedback. I applied your advices and made better tests. I also cleaned the commit if it's ok for you like this |
There was a problem hiding this comment.
Nice work, @noahfraiture! 👍🏻 I left two small suggestions (sorry, I noticed them only now), but the PR looks good. After addressing these last comments, I'll approve your PR so that one of the maintainers can take a look themselves.
Co-authored-by: Federico <me@azraelsec.sh>
This is good suggestion, I should have made them in the first place. Done and tested |
There was a problem hiding this comment.
Thanks @noahfraiture, looks good to me 💪🏻 I would have suggested you to fix up the previous commits instead of adding a new one, but let's see what the maintainers think. Thanks for contributing!
@stefanhaller @jesseduffield this PR looks good to me - feel free to look at this when you have space for it 🙏🏻
Hello,
This PR add merge --squash. A PR already exist #3130, but the author abandoned it, so I remake it.
I modified to fit most of the comment made except this one https://github.com/jesseduffield/lazygit/pull/3130/files#r1404808121. I didn't find an existing example and thus didn't know to modify the code to fit it to the new way of doing things.
There's still the choice box to commit or not to do as discussed #3130 (comment). I'll do it when I have time, I first need to read the code to see how it really works.
Also only english has been made for now.
go generate ./...)docs/Config.md) have been updated if necessary