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

[Erik Duveblad]

On Fri, 1 Dec 2023 13:58:55 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Erik Duveblad has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge 'master'
>>  - Reviewer feedback
>>  - Merge master
>>  - optimize
>>  - final review
>>  - refactor
>>  - whitespace
>>  - do not throw from checkStatus
>>  - Merge branch 'master' into pr-correct-jcheck-conf
>>  - Fix .jcheck/conf parsing in multiple places
>>  - ... and 1 more: https://git.openjdk.org/skara/compare/a9727b65...f22f4878
>
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1253:
> 
>> 1251: 
>> 1252:                         var commitJCheckConf = checkablePullRequest.parseJCheckConfiguration(hash);
>> 1253:                         var commitVisitor = checkablePullRequest.createVisitor(commitJCheckConf);
> 
> I think these should be moved inside the if block below.

Agree, fixed in latest

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1254:
> 
>> 1252:                         var commitJCheckConf = checkablePullRequest.parseJCheckConfiguration(hash);
>> 1253:                         var commitVisitor = checkablePullRequest.createVisitor(commitJCheckConf);
>> 1254:                         if (isJCheckConfUpdatedInMergePR) {
> 
> Shouldn't this be conditional on the repo using an external jcheck conf?

Yep, agree, fixed in latest

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1304:
> 
>> 1302:                 // then we won't use the one in the repo anyway.
>> 1303:                 if (workItem.bot.confOverrideRepository().isEmpty() &&
>> 1304:                     (isFileUpdated(Path.of(".jcheck").resolve("conf"), localHash) || isJCheckConfUpdatedInMergePR)) {
> 
> Style nit. I think this way of creating a path looks a bit overly complex. I would suggest either just `Path.of(".jcheck/conf")` or split the elements like this `Path.of(".jcheck", "conf")`.

Sure, fixed in latest

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1308:
> 
>> 1306:                     var localJCheckConf = checkablePullRequest.parseJCheckConfiguration(localHash);
>> 1307:                     var localVisitor = checkablePullRequest.createVisitor(localJCheckConf);
>> 1308:                     log.info("Run jcheck against localHash with JCHeck configuration from localHash");
> 
> Suggestion:
> 
>                     log.info("Run JCheck against localHash with configuration from localHash");

Fixed in latest

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1320:
> 
>> 1318:             }
>> 1319: 
>> 1320:             var confFile = localRepo.lines(Path.of(".jcheck").resolve("conf"), localHash);
> 
> Same style nit as above.

Fixed in latest

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

PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412238698
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412238335
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412238564
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412238180
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412238041