Skip to content
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

Jump to bottom

No way to close a PR. #63

Closed
Tracked by #61
sven-of-cord opened this issue Jun 13, 2022 · 2 comments · Fixed by #109
Closed
Tracked by #61

No way to close a PR. #63

sven-of-cord opened this issue Jun 13, 2022 · 2 comments · Fixed by #109

Comments

@sven-of-cord
Copy link
Contributor

sven-of-cord commented Jun 13, 2022 •
edited
Loading

@joneshf reported this in #61

  • Not entirely sure what this maps to in arc, maybe arc close-revision?
@sven-of-cord sven-of-cord mentioned this issue Jun 13, 2022
Closed
10 tasks
@joneshf
Copy link
Contributor

joneshf commented Jun 25, 2022

I was thinking about giving this a whirl. Assuming that's fine, I had a couple of questions before actually trying it:

  • What would the command be called? Would it be spr close-revision to match arc? Or would it deviate from arc with something like spr close or another thing?

  • Having never used arc in practice, I don't know all the ins and outs of what arc close-revision does. What all needs to happen in this command? It seems like:

    1. Close PR.
    2. Delete branch on GitHub.
    3. Delete branch in local repo.
    4. If it has a base branch:
      1. Delete base branch on GitHub.
      2. Delete base branch in local repo.

    Is that too much stuff? Is there something else I might've missed?

@sven-of-cord
Copy link
Contributor Author

Nice, this would be a welcome addition indeed.

To be honest, I am no arc expert at all. I did use it every day at work, but I had my two to three things I did with it and don't know the details about all the other commands.

I think spr close would be a good name. "Revision" isn't GitHub lingo, and even when I still actively used Phabricator, I found the diff/revision nomenclature not well designed. Revision numbers began with a capital D (like D123), which meant that people said diff all the time, when they actually meant revision. Was all a bit muddled.

I think steps iii and iv.b don't need to be implemented. They are no local branches, just the references to remote branches (refs/remotes/origin/spr/...), and those will get removed by the "git push --delete ...".

Other than that: looks good!

@joneshf joneshf mentioned this issue Jul 7, 2022
Merged
11 tasks
sven-of-cord pushed a commit that referenced this issue Jul 12, 2022
@joneshf
Add a close command
ccb61bb 
We want to be able to close a PR from `spr`.

This follows a lot of what happens in the other commands: we grab the
commit, find its related PR, close it, and delete any branches.

We also give it a `--all` option so that all PRs in a stack can be
closed at once.

Closes: #63

Test Plan:
There are three scenarios we want to check: closing a PR based on the
trunk branch, closing a PR based on a non-trunk branch, and closing a
stack of PRs.

## Trunk branch

1. Create a PR with `spr diff`.
1. Run `spr close`.
    - [ ] The PR should be closed
    - [ ] The head branch should be deleted on GitHub.
    - [ ] The commit message should no longer have the "Pull Request" or
        "Reviewed By" sections.

## Non-trunk branch

1. Create one PR with `spr diff`.
1. Create a second PR with `spr diff`.
1. Run `spr close` on the second PR.
    - [ ] The PR should be closed
    - [ ] The head branch should be deleted on GitHub.
    - [ ] The base branch should also be deleted on GitHub.
    - [ ] The commit message should no longer have the "Pull Request" or
        "Reviewed By" sections.

## Stack of PRs

1. Create one PR with `spr diff`.
1. Create a second PR with `spr diff`.
1. Run `spr close --all` on the second PR.
    - [ ] Each PR should be closed
    - [ ] Each head branch should be deleted on GitHub.
    - [ ] Each base branch should also be deleted on GitHub.
    - [ ] Each commit message should no longer have the "Pull Request"
        or "Reviewed By" sections.

Reviewers: sven-of-cord
    
Reviewed By: sven-of-cord

Pull Request: #109
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add a close command joneshf/spr
2 participants
@joneshf @sven-of-cord