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