Skip to content

Always fill out PR title after conflict resolution#20

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.

Sign up for GitHub

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

Open
webknjaz wants to merge 1 commit into
base:main
Choose a base branch
from
Open

Always fill out PR title after conflict resolution #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions cherry_picker/cherry_picker.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -277,7 +277,7 @@ def amend_commit_message(self, cherry_pick_branch):
click.echo(cpe.output)
return updated_commit_message

def push_to_remote(self, base_branch, head_branch, commit_message=""):
def push_to_remote(self, base_branch, head_branch, commit_message):

@jaracojaracoJun 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is removing the default necessary?

@MariattaMariattaOct 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webknjaz can we keep the original behavior and let the default commit message?

@webknjazwebknjazOct 3, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember, it was over a year ago. But I think I wanted to prevent situations when a caller would accidentally leave it empty. Why would one want this behavior to be implicit? If a caller wants an empty title, they should state that explicitly instead of relying on the magical empty value. Besides, it's passed to create_gh_pr() without changes, which already enforces the same.

@webknjazwebknjazJan 26, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, I've checked that the only known consumer does not call this method directly: https://github.com/python/miss-islington/blob/23841c0b44b84981d0db055e7eb0c61a532b5aa4/miss_islington/tasks.py#L128. And there's probably no other public API user in the wild.

Having no default here is intended to force the callers to always compute the PR title as opposed to allowing it to be empty when forgotten.

""" git push <origin> <branchname> """
set_state(WORKFLOW_STATES.PUSHING_TO_REMOTE)

Expand DownExpand Up@@ -468,7 +468,10 @@ def continue_cherry_pick(self):
]
subprocess.check_output(cmd, stderr=subprocess.STDOUT)

self.push_to_remote(base, cherry_pick_branch)
self.push_to_remote(
base, cherry_pick_branch,
updated_commit_message,
)

self.cleanup_branch(cherry_pick_branch)

Expand Down
6 changes: 3 additions & 3 deletions cherry_picker/test.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -648,7 +648,7 @@ def test_push_to_remote_fail(tmp_git_repo_dir):
with mock.patch("cherry_picker.cherry_picker.validate_sha", return_value=True):
cherry_picker = CherryPicker("origin", "xxx", [])

cherry_picker.push_to_remote("master", "backport-branch-test")
cherry_picker.push_to_remote("master", "backport-branch-test", "")
assert get_state() == WORKFLOW_STATES.PUSHING_TO_REMOTE_FAILED


Expand All@@ -659,7 +659,7 @@ def test_push_to_remote_interactive(tmp_git_repo_dir):
with mock.patch.object(cherry_picker, "run_cmd"), mock.patch.object(
cherry_picker, "open_pr"
), mock.patch.object(cherry_picker, "get_pr_url", return_value="https://pr_url"):
cherry_picker.push_to_remote("master", "backport-branch-test")
cherry_picker.push_to_remote("master", "backport-branch-test", "")
assert get_state() == WORKFLOW_STATES.PR_OPENING


Expand All@@ -671,7 +671,7 @@ def test_push_to_remote_botflow(tmp_git_repo_dir, monkeypatch):
with mock.patch.object(cherry_picker, "run_cmd"), mock.patch.object(
cherry_picker, "create_gh_pr"
):
cherry_picker.push_to_remote("master", "backport-branch-test")
cherry_picker.push_to_remote("master", "backport-branch-test", "")
assert get_state() == WORKFLOW_STATES.PR_CREATING


Expand Down