RFR: SKARA-1096: New command and label for JEPs, similar to CSR [v8]

[Magnus Ihse Bursie]

On Mon, 18 Apr 2022 17:04:51 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch implements the features about the `/jep` command and JEP bot. The detailed designs and discussions are listed at [1][2].
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>> 
>> [1] https://mail.openjdk.java.net/pipermail/skara-dev/2021-December/005481.html
>> [2] https://mail.openjdk.java.net/pipermail/skara-dev/2022-March/005770.html
>
> Guoxiong Li has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Redo: Change labels after comment.
>  - Revert: Change labels after comment.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java line 38:

> 36:     default void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command,
> 37:                         List<Comment> allComments, PrintWriter reply, List<String> labelsToAdd, List<String> labelsToRemove) {
> 38:     }

I keep coming back to this in my reviewing. It would be better if we did not have to update all those other places where handle() is implemented. 

I might be thinking wrong here, but I believe that you should be able to keep the original `handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply)` method, and instead add the new `handle` (with `List<String> labelsToAdd, List<String> labelsToRemove` as a default method, with the implementatation

handle(bot, pr, censusInstance, scratchPath, command, allComments, reply);


This way, the commands that need the new label handling mechanism can implement the new handle override with the extended argument list. All other commands needs no change.

Once again, I don't feel 100% confident I'm thinking correctly here, but it seems to me like it would work, and be a better solution.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 62:

> 60:                 The project prefix of the issue ID (`JDK-` in the above examples) is optional.
> 61:                 But the `JEP-` or `jep-` prefix is required when you provide a JEP ID.
> 62:                 And the issue type must be `JEP`.

Suggestion:

                The project prefix (`JDK-` in the above examples) is optional if you use an issue ID. 
                The issue type in that case must be `JEP`.
                The `JEP-` or `jep-` prefix is required if you instead provide a JEP ID.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 68:

> 66:     private Optional<Issue> getJepIssue(String args, PullRequestBot bot) {
> 67:         Optional<Issue> jbsIssue;
> 68:         if (args.startsWith("jep-") || args.startsWith("JEP-")) {

Suggestion:

        if (args.toUpperCase().startsWith("JEP-")) {


Then variations like "Jep-123" is also accepted.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 98:

> 96:             }
> 97:             reply.println(unneededMarker);
> 98:             reply.println("determined that the [JEP](https://openjdk.java.net/jeps) request " +

Suggestion:

            reply.println("determined that the JEP request " +

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 150:

> 148:     @Override
> 149:     public String description() {
> 150:         return "require a JDK Enhancement Proposal for this pull request";

Suggestion:

        return "require a JDK Enhancement Proposal (JEP) for this pull request";

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

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