RFR: 282: Make the review requirements more explicit in the progress list

[Guoxiong Li]

On Tue, 26 Apr 2022 18:12:29 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>>> To do this the TooFewReviewersIssue would need to hold the requirements string from the configuration, but I think it makes enough sense for it to do that. ReviewersCheck has access to it and can provide it to the constructor.
>> 
>> I tried this way at first when I solved this issue. But then I found the ReviewersCheck doesn’t always return the TooFewReviewersIssue which means the message may miss sometimes. And if we return the message in all the issues of the ReviewersCheck, the message may also miss when the ReviewersCheck passes without any issue.
>
>> I tried this way at first when I solved this issue. But then I found the ReviewersCheck doesn’t always return the TooFewReviewersIssue which means the message may miss sometimes. And if we return the message in all the issues of the ReviewersCheck, the message may also miss when the ReviewersCheck passes without any issue.
> 
> Right, this would only work when the checkbox isn't checked.
> 
> My biggest issue here is that you are storing mutable state in `CheckablePullRequest`. That class is currently immutable.
> 
> Here is what I would suggest instead. Still move the logic for generating the reviewers description string to ReviewersConfiguration. Add a field to `PullRequestCheckIssueVisitor` for holding a Jcheck configuration. Set this field from `CheckablePullRequest::executeChecks`. The visitor is already a class that carries a mutable state, so this is kind of ok. Then in `PullRequestCheckIssueVisitor::get*Checks` explicitly modify the description for ReviewersCheck in the returned map. I would recommend a new private method that wraps the `Check::description` call and extracts the reviewer information from the conf field.

@erikj79 Thanks for the review.

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

PR: https://git.openjdk.java.net/skara/pull/1305