Skip to content

Conversation

@cleithner-comcast
Copy link
Contributor

The "abandon" command documents that it
closes PRs tracked by the stack; however,
it does not do that. This commit adds
an abandon_pr routine and calls it during
the abandon command for each entry in
the stack.

The "abandon" command documents that it
closes PRs tracked by the stack; however,
it does not do that. This commit adds
an abandon_pr routine and calls it during
the abandon command for each entry in
the stack.
Copy link
Collaborator

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

Thank you for the contributions! I left a couple of comments on the implementation itself, but regarding this PR in particular I also have a high level question - do we actually need it provided #27 lands? IIUC github automatically closes the PR if its branch is closed. At least that's what I see in my testing - if I apply #27 I don't need this change to get the correct behavior.

)

def abandon_pr(e: StackEntry):
log(b("Abandoning ") + e.pprint(), level=2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pprint takes one argument (that controls whether we're printing PR as links in the terminal):

Suggested change
log(b("Abandoning ") + e.pprint(), level=2)
log(b("Abandoning ") + e.pprint(False), level=2)

def abandon_pr(e: StackEntry):
log(b("Abandoning ") + e.pprint(), level=2)

run_shell_command(["gh", "pr", "close", e.pr])
Copy link
Collaborator

Choose a reason for hiding this comment

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

e.pr can be empty (e.g. if a new commit has been added to the stack), so we should handle it here to prevent a crash.

input=pr_body.encode(),
)

def abandon_pr(e: StackEntry):
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more comment: the code is organized in sections corresponding to top-level command (LAND/VIEW/SUBMIT/ABANDON). So this function probably should be moved to the "ABANDON" section (after line 1040).

@cleithner-comcast
Copy link
Contributor Author

Thank you for the contributions! I left a couple of comments on the implementation itself, but regarding this PR in particular I also have a high level question - do we actually need it provided #27 lands? IIUC github automatically closes the PR if its branch is closed. At least that's what I see in my testing - if I apply #27 I don't need this change to get the correct behavior.

I'll investigate this as soon as I can. If it is still needed, I'll address comments.

@cleithner-comcast
Copy link
Contributor Author

Thank you for the contributions! I left a couple of comments on the implementation itself, but regarding this PR in particular I also have a high level question - do we actually need it provided #27 lands? IIUC github automatically closes the PR if its branch is closed. At least that's what I see in my testing - if I apply #27 I don't need this change to get the correct behavior.

I'll investigate this as soon as I can. If it is still needed, I'll address comments.

You are correct, when I abandoned a PR and the remote is deleted, the PR auto-closes. I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants