RFR: 2082: Use correct .jcheck/conf in AdditionalConfiguration.java [v3]

[Erik Duveblad]

On Fri, 3 Nov 2023 14:07:34 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Erik Duveblad has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   optimize
>
> Overall I really like this change. It makes dealing with jcheck configs a lot clearer.

@erikj79 and @zhaosongzs I'm finally looping back to this and _think_ I addressed all or your comments and suggestions. Please have a look at this PR again, I think it is shaping up quite niceliy.

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1272:
> 
>> 1270:             }
>> 1271: 
>> 1272:             var visitor = checkablePullRequest.createVisitor(targetJCheckConf, localHash);
> 
> Supplying the `localHash` here seems like a dummy. It's not used for anything, because the overriding conf will be used regardless, but the current API requires a valid hash here. It took me a while and a bit of confusion before I realized this. Would it be possible to avoid that by getting rid of the unused parameter?

I agree, fixed in latest version

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java line 212:
> 
>> 210:     }
>> 211: 
>> 212:     Optional<JCheckConfiguration> parseJCheckConfiguration(Hash hash) throws IOException {
> 
> Every caller of this method is throwing IllegalStateException if jcheck conf isn't found. Did you consider just throwing here and not return Optional? I realize there is a bit more information in the exception message, but on the other hand, whoever reads it will have a stacktrace anyway.

I changed the code to throw in the latest version, good suggestion!

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java line 228:
> 
>> 226:     }
>> 227: 
>> 228:     void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor, JCheckConfiguration conf) throws IOException {
> 
> The parameter name `localHash` threw me off for a bit, maybe just `hash` in this context, unless I'm missing some context?

Sure, renamed it t to `hash` in the latest version

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

PR Comment: https://git.openjdk.org/skara/pull/1578#issuecomment-1834473140
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1411207011
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1411207366
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1411207700