RFR: 1288: Adjust the implementation of the 'reviewers' command to match the documentation
Erik Joelsson
erikj at openjdk.java.net
Mon Mar 28 20:45:20 UTC 2022
On Wed, 15 Dec 2021 12:36:24 GMT, Guoxiong Li <gli at openjdk.org> wrote:
> Hi all,
>
> The `reviewers` command [documentation](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/reviewers) states:
>
>> Description
>> Require that at least N users with given role (defaults to Author) review the pull request before it can be integrated. The requirements are in addition to the ones specified by the .jcheck/conf file.
>
> But the implementation doesn't match the documentation now, especially the bug reported at [JDK-PULL-6752](https://github.com/openjdk/jdk/pull/6752) ([SKARA-1288](https://bugs.openjdk.java.net/browse/SKARA-1288) also recorded this bug).
>
> This patch fixes the bug and revises the code to match the documentation.
>
> The logic has small change(Please see the following code). In the past, the reviewer number of the role will be the sum of the `updatedLimits.get(r)` and `remainingAdditional`. But I don't know it meets our expectation. Because the higher roles can reduce the `remainingAdditional`, I think the remaining number should be only set to `updatedLimits` but not added to `updatedLimits`. This matches the documentation `Require that at least N users with given role (defaults to Author) review the pull request`, especially the key work `at least`.
>
>
> - updatedLimits.replace(r, updatedLimits.get(r) + remainingAdditional);
> + if (remainingAdditional > updatedLimits.get(r)) {
> + // Set the number for the lower roles to remove.
> + remainingRemovals = remainingAdditional - updatedLimits.get(r);
> + // The new number of the reviewers should larger or equal than the requirement of the '.jcheck/conf' file.
> + // Because the '.jcheck/conf' file means the minimal reviewer requirement.
> + updatedLimits.replace(r, remainingAdditional);
> + }
>
>
> The other code changes are listed below:
> - add negative number check.
> - remove the updated limits check. Because it is not needed after adding the negative number check.
> - adjust the reply message, always output the concrete reviewers. Because the merged logic of jcheck config and user `reviewers` command is a little hard, we should let the user know the result directly instead of guessing the result.
> - `decrease lower roles` in the method `ReviewersTracker#updatedRoleLimits` doesn't work as expected. I fix it.
> - add more test cases.
>
> Thanks for taking the time to review.
>
> Best Regards,
> -- Guoxiong
Just a few nits on language in replies and comments. Copyright years need to be updated. Otherwise this looks really good!
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java line 136:
> 134: var totalRequired = updatedLimits.values().stream().mapToInt(Integer::intValue).sum();
> 135: reply.print("The total number of required reviews for this PR, " +
> 136: "which is combined by the configuration and the command, is now set to " + totalRequired);
I would suggest:
"The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to"
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java line 66:
> 64: // Set the number for the lower roles to remove.
> 65: remainingRemovals = remainingAdditional - updatedLimits.get(r);
> 66: // The new number of the reviewers should larger or equal than the requirement of the '.jcheck/conf' file.
I would suggest moving this comment to before line 63 and express it something like this:
// The new value cannot the lower than the value in .jcheck/conf
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java line 86:
> 84: var removed = Math.max(0, originalVal - remainingRemovals);
> 85: updatedLimits.replace(r, removed);
> 86: remainingRemovals -= (originalVal - updatedLimits.get(r));
Maybe use the local variable `removed` instead of updatedLimits.get(r). I think that reads better.
-------------
PR: https://git.openjdk.java.net/skara/pull/1262
More information about the skara-dev
mailing list