RFR: 282: Make the review requirements more explicit in the progress list [v3]
Erik Joelsson
erikj at openjdk.java.net
Fri Apr 29 13:37:39 UTC 2022
On Wed, 27 Apr 2022 08:50:44 GMT, Guoxiong Li <gli at openjdk.org> wrote:
>> Hi all,
>>
>> This patch adds the explicit review requirements, such as ` (2 reviews required, with at least 1 Reviewer).`, to the progress list and adds some test cases.
>>
>> Thanks for taking the time to review.
>>
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>
> Add null check
bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java line 46:
> 44:
> 45: private static final String reviewProgressTemplate = "Change must be properly reviewed (%d reviews required, with at least %s)";
> 46: private static final String zeroReviewProgress = "Change must be properly reviewed (no reviews required)";
Please use uppercase underscore separated identifiers for constants.
Suggestion:
private static final String REVIEW_PROGRESS_TEMPLATE = "Change must be properly reviewed (%d reviews required, with at least %s)";
private static final String ZERO_REVIEW_PROGRESS = "Change must be properly reviewed (no reviews required)";
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.
-------------
PR: https://git.openjdk.java.net/skara/pull/1305
More information about the skara-dev
mailing list