feat: review command #332
feat: review command #332
Conversation
|
Hey. Thanks for the PR! But please create an issue discussion about the addition so that you don't create anything unnecessarily ;) I do think this is a good addition but will have to take some time before looking at it since the changes are extensive. Let me get back to you with a review in a few days :) |
Yeah should have commented (fixes #295). Sure take your time 👍🏻 |
| all, _ := flag.GetBool("all") | ||
| batch, _ := flag.GetString("batch") |
There was a problem hiding this comment.
Is it possible to use batch without all, or the reverse?
There was a problem hiding this comment.
Batch always refers to all pulls, so it includes --all not matter if a user would set it or not (It's describe like that in the flag helper). While you can do all without the batch option. If all is set without batch a user gets the diff for all pull requests at once.
| all, _ := flag.GetBool("all") | ||
| batch, _ := flag.GetString("batch") |
There was a problem hiding this comment.
I think batch should be named something else. state maybe? As it's called that at GitHub
There was a problem hiding this comment.
Whatever it is called, it should be converted to an "enum" here, and therefore verified to be correct, before calling out to the multigitter package.
See conflict strategy as an example
There was a problem hiding this comment.
Agreed with the enum, definitely makes sense 👍🏻
With the flag name, from a user perspective and cli tooling I would pick batch as its more obvious that something is going on which does not require user interaction which is not the case if it's called state. It could be --batch and --state=x. But thats an additional flag for no reason.
There was a problem hiding this comment.
The problem is that other settings (comments) will also be applied as a batch.
The state is called batch because it can be applied to all PRs, but comments will still be called comment, even if it will still be applied in batch.
If state where to be used, it should be required to use with --all. --all could still be used on it's own, and it would then ask for then ask for the state (and comment).
There was a problem hiding this comment.
I think --batch is more common to understand as a user that no additional follow up questions are required and it can be used silently. Might also call it silent instead.
If batch is just ditched and its called state I think it's not exactly what it's intended for.
|
Just used this new command to batch review and merge hundred pr's opened by renovate (not created using multi-gitter itself). Thats also a nice use case for this command. |
| all, _ := flag.GetBool("all") | ||
| batch, _ := flag.GetString("batch") |
There was a problem hiding this comment.
The problem is that other settings (comments) will also be applied as a batch.
The state is called batch because it can be applied to all PRs, but comments will still be called comment, even if it will still be applied in batch.
If state where to be used, it should be required to use with --all. --all could still be used on it's own, and it would then ask for then ask for the state (and comment).
lindell
force-pushed
the
master
branch
2 times, most recently
from
de07efa to
2e9b182
Compare
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Co-authored-by: Johan Lindell <[email protected]>
Co-authored-by: Johan Lindell <[email protected]>
Co-authored-by: Johan Lindell <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
Signed-off-by: Raffael Sahli <[email protected]>
|
Hi @lindell, Firstly thank you for sharing your excellent work that is this tool. |
|
Hello Thank you ! |
What does this change
Hey me again 🙈 ,
This pr adds the ability for approving pull requests supported by all platforms.
Basically reviewers now get an easy option to approve pr's made by multi-gitter (or other(s)).
Usually pull requests require approval in branch protected repos and there is currently no good way to batch approve multi pr's.
For example we collect all batch operations in a repository, a reviewer can grab the config from there and start a straight forward review process with the same tool. (Using the --batch option this could even be fully automated in a ci pipeline).
This pr adds the
reviewcommand with a couple of options:--commentfor a review comment (can also be overwritten while reviewing pr's)--allto batch review all of them. Basically review a diff output from all pr's. Useful if you have 100 pr's with a single line change where you don't want to review each individually.--batch=xApprove/Decline/Comment all targeted pull requests without asking questions--no-pagerThis pr pipes the diffs to a pager (less, more (windows), PAGER env). If no pager should be used one can dump to stdout directly using this option--include-approvedBy default approve ignores pull requests which are already approved by the reviewing person.example using vim with syntax highlighting:
What issue does it fix
Currently reviewing is hard. If you open 100 pr's with a simple change you either have to wait until people reviewed them, try to push them or whatever... There are also some workarounds like verify 2 and then batch approve everything via the api.
Notes for the reviewer
Checklist