RFR: 2331: Improve review notes [v3]

[Pavel Rappo]

On Wed, 24 Jul 2024 15:18:18 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> `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)

Let's dissect it. When you are saying you don't agree with the premise, do you mean you don't agree with this?

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

If so, please clarify. How do you want it to be?

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

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