-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Squash merge #3566
base: master
Are you sure you want to change the base?
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 |
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.
Hello 👋🏻 I left a couple of comments on your PR to discuss while you write the tests.
if checkedOutBranchName == refName { | ||
return func() error { return errors.New(self.c.Tr.CantMergeBranchIntoItself) } | ||
} |
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.
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.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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