RFR: 2015: Initial run of PR IssueBot misses issue updates

[Erik Joelsson]

On Tue, 12 Sep 2023 17:40:55 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 73:
>> 
>>> 71: 
>>> 72:     CheckWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler, ZonedDateTime triggerUpdatedAt,
>>> 73:                   boolean needsReadyCheck, boolean forceUpdate, boolean spawnedFromIssueBot, boolean initialRun) {
>> 
>> Having 4 booleans in the constructor is becoming really hard to read and understand. Perhaps we should introduce factory methods instead so we only allow for valid combinations of the booleans. Something like:
>> 
>> fromIssue
>> fromPr
>> initialRun
>> forceUpdate
>
> I was thinking that if we fix SKARA-2016 in the future, then `forceUpdate` would be removed. So three booleans is reasonable.

3 is already a lot, especially if certain combinations aren't actually valid. I was already thinking this constructor was bad before this change. 

When reading the source in intellij, it shows the parameter name at each call site, which alleviates some of the problem, but that isn't happening when reviewing on GitHub, or when merging using other tools.

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 232:
>> 
>>> 230:                         if (previousIssueMetadata.equals(currIssueMetadata) && expiresAt.isAfter(Instant.now())) {
>>> 231:                             log.finer("[Issue]Metadata with expiration time is still valid, not checking again");
>>> 232:                             return true;
>> 
>> I think this method would be easier to read if we kept all return `true`/`false` inside the conditional blocks. If we know we have a terminal state, lets be clear about it so the reader doesn't need to check the rest of the method.
>
> I removed `return true` here because we couldn't return early in this block. For the initial run case, `initialRun` would be true and `spawnedFromIssueBot` would be false. We need to check both issueMetaData and PRMetaData. 
> 
> But we could add some `return true` in `if (!spawnedFromIssueBot)` block.

Oh right, I get it now. I take back my comment.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1554#discussion_r1323472623
PR Review Comment: https://git.openjdk.org/skara/pull/1554#discussion_r1323472999