RFR: 2331: Improve review notes [v3]

[Kevin Rushforth]

On Wed, 24 Jul 2024 15:05:06 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 618:
>> 
>>> 616:                             if (!hash.get().equals(pr.headHash())) {
>>> 617:                                 if (reviewCoverage.covers(review) || !tooFewReviewers) {
>>> 618:                                     entry += (tooFewReviewers ? " ⚠️ " : " ")
>> 
>> Why are you using `tooFewReviewers` to decide whether or not to show the warning emoji. Shouldn't this check be `useStaleReviews`?
>
> `useStaleReviews` is baked into `reviewCoverage.covers(review)`:
> 
>     public boolean covers(Review review) {
>         return cachedCoverage.computeIfAbsent(review, this::covers0);
>     }
> 
>     private boolean covers0(Review review) {
>         var r = review.hash();
>         // Reviews without a hash are never valid as they referred to no longer
>         // existing commits.
>         if (r.isEmpty() || review.verdict() != Review.Verdict.APPROVED
>                 || !review.targetRef().equals(pr.targetRef())) {
>             return false;
>         }
>         if (useStaleReviews || r.get().equals(pr.headHash())) {
>             return true;
>         }
>         if (!acceptSimpleMerges) {
>             return false;
>         }
> 
> Here and elsewhere, `tooFewReviewers` is checked because it's decisive: if there are enough reviewers, no emoji or the word "required" should be used.

> If there are enough reviewers, no emoji or the word "required" should be used.

I think I now understand what you are trying to do, although I don 't think I agree with the premise. If I understand the logic (which I think I do now), here are two cases that won't do what I would expect:

1. `useStaleReviews=false` : if there are too few reviewers, simple merges will show a warning icon
2. `useStaleReviews=true` : if there are enough reviewers, all other reviews will not get the warning icon

I think 2 basically makes the warning icon useless (and 1 is a bug)

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1679#discussion_r1690006762