Don't add "reviewed" label to "trivial" PRs #38
Conversation
| pub enum Approval { | ||
| Required, | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Could probably remove this enum altogether. I can look into that in a follow-up PR.
There was a problem hiding this comment.
This is breaking a previous feature: absence of a review label didn't block a review, before.
Also, the presence of skip-review basically means that you've auto-reviewed and accepted your changes, so it's kind of a self-approval, thus a review? 😛 I don't really care, since this is really about label aesthetics at this point, but the code in the main loop becomes rather messy because of this change.
|
|
||
| if reviews.approved(review_required) { | ||
| if reviews.approved(Approval::Required) { |
There was a problem hiding this comment.
This is another subtle breaking change: the bool fed to that function before was depending on the presence of the skip review label, if it existed, AND the existence of a review label. At the very least, this parameter should be depending on the presence of the review label: Required if present, Optional if not. (It was the previous behavior before my patch, fwiw)
|
Think I'll just close this for now. If this is still something we want we can always revisit it. |
Checklist
Description of Changes
Fixes #37
The biggest change is that
pr_approvednow always checks whether the PR is actually approved or not. It doesn't consider trivial PRs to always be approved. This gives us more control over when to add the "reviewed" label since that previously was always added if a PR was approved.