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

Guoxiong Li gli at openjdk.java.net
Mon Apr 18 14:47:12 UTC 2022


On Mon, 18 Apr 2022 14:02:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> This wasn't quite how I envisioned it. I don't think we need to overload the handle method. I also don't think we should use the return value for this. We already use the reply parameter as a kind of return value, a parameter that you modify to return a new state to the caller. My idea was to just add two more parameters to the existing handle method:
>> 
>> ..., List<String> labelsToAdd, List<String> labelsToRemove)
>> 
>> The logic in `processCommand` wouldn't need to change much. It would need to instantiate two ArrayLists in the case of `isCommit==false`, and then loop through each list to process the label changes on the pr. The `pr.addComment(writer.toString())` line would need to be moved from the end to just after each of the two `handle` calls.
>
> I misunderstood your idea previously. It is better to add two more parameters to the existing handle method than overload it.
> 
> After adding two more parameters, the signatures of the method `handle` in all the commands need to be changed, too. Is it the expected behaviour? I need to confirm it, because it may change many files.
> 
> ---
> 
> The method `CommandHandler#changeLabelsAfterComment`  can be added if we want to control it explicitly.
> If the `changeLabelsAfterComment` is true, it has these three steps:
> 1. add/remove labels before comment (in the method `handle`)
> 2. comment (in the method `processCommand`)
> 3. add/remove labels after comment (in the method `processCommand`)
> 
> If the `changeLabelsAfterComment` is false, it has only two steps:
> 1. add/remove labels before comment (in the method `handle`)
> 2. comment (in the method `processCommand`)
>  
> If we choose not to add the method `CommandHandler#changeLabelsAfterComment`, we may do the test `labelsToAdd is not empty && labelsToRemove is not empty` to implicitly confirm whether the command want to change the labels after commenting. This implicit way actually make the same three steps : 
> 1. add/remove labels before comment (in the method `handle`)
> 2. comment (in the method `processCommand`)
> 3. add/remove labels after comment if existing (in the method `processCommand`)
> 
> What's your opinion, explicitly or implicitly?

Now, I think the implicit way is better and follow your idea above.

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

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


More information about the skara-dev mailing list