RFR: 2331: Improve review notes

Zhao Song zsong at openjdk.org
Thu Jul 18 20:01:13 UTC 2024


On Thu, 18 Jul 2024 19:53:49 GMT, Zhao Song <zsong at openjdk.org> wrote:

>>> 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.

It's just a suggestion, since you don't like it and I am ok with your pr, I will approve it now.

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

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


More information about the skara-dev mailing list