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