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

Erik Joelsson erikj at openjdk.java.net
Fri Apr 29 22:21:30 UTC 2022


On Fri, 29 Apr 2022 15:00:33 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersConfiguration.java line 103:
>> 
>>> 101: 
>>> 102:     public String getReviewRequirements() {
>>> 103:         return reviewRequirements;
>> 
>> I would probably have put the logic for generating this string right here in the getter. Are we ever calling this more than once? Could also generate it lazily.
>> 
>> I realize this gets a lot of testing through the ReviewersTest above, but it would make sense to have cheap and quick tests of just this functionality here in the jcheck package.
>
>> I would probably have put the logic for generating this string right here in the getter. 
> 
> Fixed. Please note the field `reviewRequirements` is not `final` now.
> 
>> I realize this gets a lot of testing through the ReviewersTest above, but it would make sense to have cheap and quick tests of just this functionality here in the jcheck package.
> 
> Added tests.

That's fine, thanks!

I now realize that you put the space and parentheses in the string here. I think this method should just return the requirements and let the visitor add the surrounding formatting.

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

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


More information about the skara-dev mailing list