initial support for --close-pr (REVIEW) #2401
Conversation
|
there are quite a few ways this could be improved (e.g., in dry-run mode, show related PRs, whether it uses archived toolchain, date of last commit and comments, etc.), but that would involve even more code duplication with not sure if the best approach would be a common close/merge function or fleshing out (almost) identical code blocks... |
| g = RestClient(GITHUB_API_URL, username=github_user, token=github_token) | ||
| pull_url = g.repos[pr_target_account][pr_target_repo].pulls[pr] | ||
| body = {'state': 'closed'} | ||
| status, data = pull_url.post(body=body) |
There was a problem hiding this comment.
There's a lot of duplication here too, maybe time for a pr_action function that gets PR# + body as arguments (and github_token as optional argument)?
There was a problem hiding this comment.
@migueldiascosta Does this suggestion still make sense, or not?
There was a problem hiding this comment.
@boegel I don't know - each case is slightly different, but I may be missing an obvious generalization
There was a problem hiding this comment.
Just took a look myself, it's indeed not entirely trivial, so let's leave it as is (for now).
| g = RestClient(GITHUB_API_URL, username=github_user, token=github_token) | ||
| pull_url = g.repos[pr_target_account][pr_target_repo].pulls[pr] | ||
| body = {'state': 'closed'} | ||
| status, data = pull_url.post(body=body) |
There was a problem hiding this comment.
@migueldiascosta Does this suggestion still make sense, or not?
migueldiascosta
force-pushed
the
close_pr
branch
from
6798f25 to
e5898ba
Compare
migueldiascosta
changed the title
|
In the context of considering auto-closing of stale PRs (e.g. through @boegelbot), there are quite a bit of good/interesting ideas/viewpoints in spack/spack#8811 . The main difference I guess is that our approach would be project-aware (or at least it could be). |
|
@migueldiascosta Needs a sync with Other than that, do you think this is ready to go? |
|
@migueldiascosta We should revisit this after #2400 gets merged, it's quite likely there will be a couple of merge conflicts to deal with... |
|
@migueldiascosta Let's sync this branch with current |
|
@migueldiascosta More conflicts, probably because of #2668? |
|
@migueldiascosta Some code cleanup in migueldiascosta#6 (tests still pass). One thing that is still missing is a dedicated test for |
code cleanup in tools/github.py
|
@migueldiascosta Tests added in migueldiascosta#7 |
add tests for close_pr and reasons_for_closing
|
lgtm, |
3 participants
No description provided.