RFR: 2331: Improve review notes

[Zhao Song]

On Thu, 18 Jul 2024 19:39:03 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1351:
>> 
>>> 1349:                 jcheckType = "target jcheck";
>>> 1350:                 var issues = checkablePullRequest.executeChecks(localHash, censusInstance, visitor, targetJCheckConf);
>>> 1351:                 tooFewReviewers = issues.stream().anyMatch(TooFewReviewersIssue.class::isInstance);
>> 
>> I am not sure if it's a good idea to change the return value of `CheckablePullRequest.executeChecks`.
>> 
>> I think you can use `visitor.getChecks()` to determine if reviewers check failed or not.
>> 
>> 
>>         var tooFewReviewers = visitor.getChecks()
>>                 .entrySet().stream()
>>                 .anyMatch(entry -> entry.getKey().contains("Change must be properly reviewed") && entry.getValue().equals(false));
>
>> I am not sure if it's a good idea to change the return value of `CheckablePullRequest.executeChecks`.
> 
> Why?
> 
>> I think you can use `visitor.getChecks()` to determine if reviewers check failed or not.
> 
> Keys from `Map<String, Boolean> getChecks()` are to be displayed to an end user; they don't seem to be for programmatic consumption. I shudder when thinking about parsing these:
> 
>     "Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))"
>     "Change must not contain extraneous whitespace"

I was just thinking we should make the minimal necessary changes.  I admit that parsing text displayed to the end user isn't best practice, but we do it in many places of SKARA.

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

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