RFR: 1357: Add Oracle copyright header format check to jcheck [v2]

[Erik Joelsson]

On Mon, 9 Dec 2024 22:51:22 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> This patch introduces a copyright format jcheck.
>> For pull requests, this check checks the format of copyright header in all modified files(if the file name matches the regex in .jcheck/conf)
>> 
>> To enable the new copyrightFormat jcheck, proper configurations are needed in .jcheck/conf. A locator, a validator are needed in the .jcheck/conf file. The locator is only used to locate the copyright line, so the regex is easier to match. If the locator could find the copyright line, then validator will be used to validate the format of the line. There is also an optional parameter "required", it indicates whether a copyright is required for each file, if true, the check will fail if the copyright is missing.
>> 
>> For example, if we want to enable a copyright format check for oracle.
>> We can define the configuration like this 
>> 
>> oracle_locator=.*Copyright (c)(.*)Oracle and/or its affiliates. All rights reserved.
>> oracle_validator=.*Copyright (c) (\d{4})(?:, (\d{4}))?, Oracle and/or its affiliates. All rights reserved. 
>> oracle_required=true
>
> Zhao Song has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - add copyright header
>  - version 2
>  - update
>  - review comment

cli/src/main/java/org/openjdk/skara/cli/JCheckCLIVisitor.java line 332:

> 330:     public void visit(CopyrightFormatIssue i) {
> 331:         if (!ignore.contains(i.check().name()) && !isLax) {
> 332:             if (!i.filesWithCopyrightFormatIssue().isEmpty()) {

No need to check empty since we are iterating over it anyway. It doesn't hurt to do it, but I think it's more readable without the extra if statement.

jcheck/src/main/java/org/openjdk/skara/jcheck/CopyrightFormatCheck.java line 52:

> 50:             return iterator();
> 51:         }
> 52:         var pattern = Pattern.compile(copyrightConf.files());

Could use a more descriptive name.
Suggestion:

        var filesPattern = Pattern.compile(copyrightConf.files());

jcheck/src/main/java/org/openjdk/skara/jcheck/CopyrightFormatCheck.java line 53:

> 51:         }
> 52:         var pattern = Pattern.compile(copyrightConf.files());
> 53:         var copyrightSingleCheckConfigurations = copyrightConf.singleCheckConfigurations();

This variable name is very long. How about just `copyrightConfigs`?

jcheck/src/main/java/org/openjdk/skara/jcheck/CopyrightFormatCheck.java line 88:

> 86:                         }
> 87:                     } catch (IOException e) {
> 88:                         throw new RuntimeException(e);

I would suggest throwing `UncheckedIOException` instead.

jcheck/src/main/java/org/openjdk/skara/jcheck/CopyrightSingleCheckConfiguration.java line 27:

> 25: import java.util.regex.Pattern;
> 26: 
> 27: public class CopyrightSingleCheckConfiguration {

Maybe move this to an inner class of `CopyrightFormatConfiguration` and shorten the name to something more manageable?

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

PR Review Comment: https://git.openjdk.org/skara/pull/1702#discussion_r1878154655
PR Review Comment: https://git.openjdk.org/skara/pull/1702#discussion_r1878160128
PR Review Comment: https://git.openjdk.org/skara/pull/1702#discussion_r1878163519
PR Review Comment: https://git.openjdk.org/skara/pull/1702#discussion_r1878167515
PR Review Comment: https://git.openjdk.org/skara/pull/1702#discussion_r1878177156