Open levibostian opened 5 months ago
I would love to see this feature!
@levibostian I think the branch name will be from_branch
, based on the mocked pull request object created here in mockclient. Unfortunately that will be the branch name for every PR because the mocks don't handle different branches.
I tried replacing your ??? with from_branch
but there's still quite a few test failures. I think because the expectations in other tests are not expecting the branch deletions: output.log
Thanks for trying someone out! I appreciate the help.
Do you have a commit or branch you could share with me on the changes you made? It could help me test out your idea faster.
Yup, @levibostian, here's a patch you can git apply
:
From 43472885dda7d7ed03aa22bb04460b1e6a7a89aa Mon Sep 17 00:00:00 2001
From: Andrew Szczepanski <aszczepanski@financeit.io>
Date: Tue, 21 May 2024 15:36:45 -0400
Subject: [PATCH] Updating ??? with from_branch
---
spr/spr_test.go | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/spr/spr_test.go b/spr/spr_test.go
index 7540358..6cd1b1a 100644
--- a/spr/spr_test.go
+++ b/spr/spr_test.go
@@ -306,13 +306,13 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE)
githubmock.ExpectCommentPullRequest(c1)
githubmock.ExpectClosePullRequest(c1)
- gitmock.ExpectDeleteBranch('???') // Not sure where to find the branch name?
+ gitmock.ExpectDeleteBranch("from_branch")
githubmock.ExpectCommentPullRequest(c2)
githubmock.ExpectClosePullRequest(c2)
- gitmock.ExpectDeleteBranch('???')
+ gitmock.ExpectDeleteBranch("from_branch")
githubmock.ExpectCommentPullRequest(c3)
githubmock.ExpectClosePullRequest(c3)
- gitmock.ExpectDeleteBranch('???')
+ gitmock.ExpectDeleteBranch("from_branch")
s.MergePullRequests(ctx, nil)
lines = strings.Split(output.String(), "\n")
assert.Equal("MERGED 1 : test commit 1", lines[0])
--
2.45.1
or pull down https://github.com/Skipants/spr/tree/delete-closed-branches
Nice work @levibostian! Feedback:
Sorry about the long delay guys. I agree with @chriscz, lets add a configuration knob for this, off by default so we maintain the same logic, and then can also add a separate unit test for this case.
Attempt at implementing this feature.
I did get stuck on writing tests for this feature as I could not determine how to get the branch name from the code inside of
spr/spr_test.go
.Feel free to edit the code directly in this PR if you are interested in getting it merged in.