Allow for deleting and restoring branch associated with PR #1784

austinsasko · 2020-12-14T15:55:50Z

Adding logic for deleting/restoring branches associated with a PR, as well as forcing a delete.
Fixes #580

s-t-e-v-e-n-k

This is looking like a great start, I have some concerns I have outlined.

github/PullRequest.py

tests/PullRequest.py

codecov-io · 2020-12-24T00:17:37Z

Codecov Report

Merging #1784 (16b2544) into master (34d097c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1784   +/-   ##
=======================================
  Coverage   98.76%   98.76%           
=======================================
  Files          51       51           
  Lines        2677     2677           
=======================================
  Hits         2644     2644           
  Misses         33       33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34d097c...16b2544. Read the comment docs.

austinsasko · 2020-12-24T01:29:04Z

@s-t-e-v-e-n-k I am trying to figure out why code coverage decreased on a file that I did not interface with or modify any tests that interacted with it. Any ideas?

Update: It appears the changes in #1797 will bring the code coverage back to up to the place it should be. I believe the latest pyJWT being released yesterday is causing codecov to not reach that line in MainClass.py -- as long as that PR (1797) is merged before this one, I think that codecoverage wont be affected. Apologies for any spam that I caused by troubleshooting that.

* Fixes PyGithub#580 -- adding logic for deleting/restoring branches associated with a PR, as well as forcing a delete. Automated tests added too * Adding black formatting and cleansing PII to the best of my ability * CI failure fix - isort formatting, import moved up in the file * fixing additional requests and adding docstrings * Accounting for CI test for pyupgrade

austinsasko · 2020-12-28T19:38:59Z

As believed, now that the other PR has been merged (and upon me pulling latest master into my branch) this one is ready to be merged as well with no issues :)
@s-t-e-v-e-n-k do you need anything else from me?

Thanks

austinsasko · 2021-02-05T22:32:04Z

@s-t-e-v-e-n-k @sfdye Is there anything else in this PR needing changes?

s-t-e-v-e-n-k

This is looking like a good start, I have some concerns inline.

github/PullRequest.py

tests/PullRequest.py

austinsasko · 2021-03-26T19:41:39Z

@s-t-e-v-e-n-k 👍 applied your recommendations

s-t-e-v-e-n-k

Thanks for your changes, you're making good progress, some more changes required, though.

github/PullRequest.py

tests/PullRequest.py

austinsasko · 2021-04-05T16:25:47Z

@s-t-e-v-e-n-k removing import github breaks the CI tests. I re-added it and the tests are passing.

Seluj78 · 2021-04-24T18:37:31Z

Hi ! any news on this ? I'd love to implement it on https://github.com/AFPy/PyDocTeur :)

austinsasko · 2021-05-25T14:13:03Z

@s-t-e-v-e-n-k

austinsasko · 2021-06-15T19:28:06Z

@sfdye anything we can do here?

austinsasko · 2021-12-31T08:29:07Z

@s-t-e-v-e-n-k @sfdye

austinsasko · 2022-01-16T03:18:12Z

@s-t-e-v-e-n-k @sfdye who maintains this project currently, if anyone?

austinsasko · 2022-04-12T05:06:59Z

@s-t-e-v-e-n-k @sfdye hoping to make one last ditch effort to follow up

austinsasko · 2023-06-05T14:35:29Z

@JLLeitschuh @EnricoMi
Are you guys able to review this?

Thanks

JLLeitschuh · 2023-06-05T14:38:29Z

github/PullRequest.py

@@ -835,12 +835,35 @@ def is_merged(self):
        status, headers, data = self._requester.requestJson("GET", self.url + "/merge")
        return status == 204

+    def restore_branch(self):


Missing .pyi file API addition

JLLeitschuh · 2023-06-05T14:38:44Z

github/PullRequest.py

    def merge(
        self,
        commit_message=github.GithubObject.NotSet,
        commit_title=github.GithubObject.NotSet,
        merge_method=github.GithubObject.NotSet,
        sha=github.GithubObject.NotSet,
+        deletebranch=False,


Missing .pyi file API modification

Suggested change

deletebranch=False,

delete_branch=False,

JLLeitschuh · 2023-06-05T14:38:49Z

github/PullRequest.py

+            f"refs/heads/{self.head.ref}", sha=self.head.sha
+        )
+
+    def delete_branch(self, force=False):


Missing .pyi file API addition

EnricoMi · 2023-06-28T07:39:50Z

github/PullRequest.py

+        if deletebranch:
+            self.delete_branch()
+


To be honest, I don't like adding this side-effect here. Given we have delete_branch, it is very easy for users to delete the branch with a single line after merge returned. When more pull requests remain, delete_branch raises an error and call site does not get the successful PullRequestMergeStatus response.

EnricoMi · 2023-06-28T07:45:19Z

github/PullRequest.py

+                raise AttributeError(
+                    "PRs referencing this branch remain. Not deleting the branch"
+                )


I would not call this AttributeError, because the values of the attributes are right, This is more a RuntimeError, as delete_branch(force=False) should not be called as long as there exist pull requests.

Suggested change

raise AttributeError(

"PRs referencing this branch remain. Not deleting the branch"

)

raise RuntimeError(

"This branch is referenced by open pull requests, set force=True to delete this branch."

)

EnricoMi · 2023-06-28T07:46:34Z

github/PullRequest.py

+    def delete_branch(self, force=False):
+        """
+        Convenience function that calls :meth:`GitRef.delete`
+        :rtype: bool


Please add some notes on the semantics of force.

EnricoMi · 2023-06-28T07:54:07Z

tests/PullRequest.py

+        self.assertTrue(
+            self.delete_restore_repo.get_branch(self.delete_restore_pull.head.ref)
+        )
+        self.assertIs(self.delete_restore_pull.delete_branch(force=False), None)


Why this assertion?

Suggested change

self.assertIs(self.delete_restore_pull.delete_branch(force=False), None)

self.delete_restore_pull.delete_branch(force=False)

treee111 · 2024-01-17T09:48:15Z

@austinsasko seams you have to integrate the latest changes from main into your branch. Also some review comments are waiting to be integrated. Would be nice to see further progress here 🔢

austinsasko changed the title ~~Fixes #580~~ Implementing logic for deleting and restoring a branch associated with a PR Dec 14, 2020

s-t-e-v-e-n-k requested changes Dec 16, 2020

View reviewed changes

github/PullRequest.py Outdated Show resolved Hide resolved

github/PullRequest.py Outdated Show resolved Hide resolved

github/PullRequest.py Outdated Show resolved Hide resolved

tests/PullRequest.py Outdated Show resolved Hide resolved

tests/PullRequest.py Outdated Show resolved Hide resolved

austinsasko requested a review from s-t-e-v-e-n-k December 24, 2020 00:15

austinsasko force-pushed the feature/adding-delete-restore-logic branch 3 times, most recently from a254363 to fee99a2 Compare December 24, 2020 01:02

austinsasko closed this Dec 24, 2020

austinsasko force-pushed the feature/adding-delete-restore-logic branch from fee99a2 to f299699 Compare December 24, 2020 01:25

austinsasko reopened this Dec 24, 2020

Seluj78 mentioned this pull request Dec 27, 2020

Delete branch after merging AFPy/PyDocTeur#12

Closed

austinsasko and others added 2 commits December 28, 2020 14:34

fixing docstring for delete_branch

16b2544

austinsasko force-pushed the feature/adding-delete-restore-logic branch from dd0718d to 16b2544 Compare December 28, 2020 19:35

s-t-e-v-e-n-k requested changes Mar 24, 2021

View reviewed changes

austinsasko added 2 commits March 26, 2021 15:30

Adding optimizations and improvements based on stevenk comments

93c718f

Fixing lint issues

84e97f4

s-t-e-v-e-n-k requested changes Mar 27, 2021

View reviewed changes

Adding improvements from PR discussion

3c763af

austinsasko closed this Mar 29, 2021

austinsasko deleted the feature/adding-delete-restore-logic branch March 29, 2021 17:32

austinsasko restored the feature/adding-delete-restore-logic branch March 29, 2021 17:33

austinsasko reopened this Mar 29, 2021

austinsasko added 3 commits March 29, 2021 13:35

Adding improvements from PR discussion

6d6f685

Adding improvements from PR discussion

051b9c9

Testing import GH

6b9f753

adding space for linter

49e3a8d

austinsasko requested a review from s-t-e-v-e-n-k April 6, 2021 17:21

austinsasko mentioned this pull request Jan 3, 2023

Delete/restore branch associated with a pull request #580

Open

JLLeitschuh reviewed Jun 5, 2023

View reviewed changes

EnricoMi changed the title ~~Implementing logic for deleting and restoring a branch associated with a PR~~ Allow for deleting and restoring branch associated with PR Jun 28, 2023

EnricoMi reviewed Jun 28, 2023

View reviewed changes

github-actions bot added the has conflicts label Jul 18, 2023

EnricoMi removed the has conflicts label Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for deleting and restoring branch associated with PR #1784

Allow for deleting and restoring branch associated with PR #1784

austinsasko commented Dec 14, 2020 •

edited

s-t-e-v-e-n-k left a comment

codecov-io commented Dec 24, 2020 •

edited

austinsasko commented Dec 24, 2020 •

edited

austinsasko commented Dec 28, 2020 •

edited

austinsasko commented Feb 5, 2021

s-t-e-v-e-n-k left a comment

austinsasko commented Mar 26, 2021

s-t-e-v-e-n-k left a comment

austinsasko commented Apr 5, 2021

Seluj78 commented Apr 24, 2021

austinsasko commented May 25, 2021

austinsasko commented Jun 15, 2021

austinsasko commented Dec 31, 2021

austinsasko commented Jan 16, 2022

austinsasko commented Apr 12, 2022

austinsasko commented Jun 5, 2023

JLLeitschuh Jun 5, 2023

JLLeitschuh Jun 5, 2023

EnricoMi Jun 28, 2023

JLLeitschuh Jun 5, 2023

EnricoMi Jun 28, 2023

EnricoMi Jun 28, 2023

EnricoMi Jun 28, 2023

EnricoMi Jun 28, 2023

treee111 commented Jan 17, 2024

	self.assertIs(self.delete_restore_pull.delete_branch(force=False), None)
	self.delete_restore_pull.delete_branch(force=False)

Allow for deleting and restoring branch associated with PR #1784

Are you sure you want to change the base?

Allow for deleting and restoring branch associated with PR #1784

Conversation

austinsasko commented Dec 14, 2020 • edited

s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 24, 2020 • edited

Codecov Report

austinsasko commented Dec 24, 2020 • edited

austinsasko commented Dec 28, 2020 • edited

austinsasko commented Feb 5, 2021

s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

austinsasko commented Mar 26, 2021

s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

austinsasko commented Apr 5, 2021

Seluj78 commented Apr 24, 2021

austinsasko commented May 25, 2021

austinsasko commented Jun 15, 2021

austinsasko commented Dec 31, 2021

austinsasko commented Jan 16, 2022

austinsasko commented Apr 12, 2022

austinsasko commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treee111 commented Jan 17, 2024

austinsasko commented Dec 14, 2020 •

edited

codecov-io commented Dec 24, 2020 •

edited

austinsasko commented Dec 24, 2020 •

edited

austinsasko commented Dec 28, 2020 •

edited