okumin commented on PR #6125:
URL: https://github.com/apache/hive/pull/6125#issuecomment-3404283396
@InvisibleProgrammer
Thanks for your review!
> My run changed only half of the files mentioned in this PR. It is a
significant difference. Is there an additional script that collects files with
incorrect header format?
As I mentioned, some changes are hand-maded. That's because it is overkill
to write a bash script for a pattern involving only a couple of files. See the
description of this PR for the details.
### How to reformat
I ran the following script to convert `\A\/\*\*$` into `\A\/\\*$`.
```
git ls-files '*.java' | grep -v 'src/gen/thrift' | grep -v '^iceberg/' \
| while read -r f; do
perl -0777 -i -pe 's/\A[ \t]*\/\*\*(\R)/\/*$1/' "$f"
done
```
As the number of the other patterns was not so huge, everything else has
been hand-maded.
> What is the reason of the suppressions? The checkstyle run for me went
well without them.
You added the new check to the existing `checkstyle.xml`. [Our current
configuration does not raise an error but shows a
warning](https://github.com/apache/hive/blob/ad55d58eadd6c1aabc7fed51e25dfffe158a44f6/checkstyle/checkstyle.xml#L54).
Please follow the following part in the description, or you can run `mvn
checkstyle:checkstyle-aggregate` and manually see
`./target/reports/checkstyle-aggregate.html`. I recommend my way because the
latter method shows you a million warnings unrelated to the header issue.
### How was this patch tested?
I put the following configuration on `checkstyle/checkstyle.xml`,
`standalone-metastore/checkstyle/checkstyle.xml`, and
`storage-api/checkstyle/checkstyle.xml. After that, I ran `mvn
checkstyle:check`.
```xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.2//EN"
"http://www.puppycrawl.com/dtds/configuration_1_2.dtd";>
<module name="Checker">
<module name="RegexpHeader">
<property name="headerFile"
value="${config_loc}/asf.header.incremental"/>
<property name="fileExtensions" value="java"/>
<property name="multiLines" value="2"/>
</module>
</module>
```
> Why do we need the configuration exclusion at storage-api?
Do you mean [this
change](https://github.com/apache/hive/blob/be6223437da47906f307aceda2e1e5319a9a8d34/storage-api/pom.xml#L221-L235)?
It excludes `checkstyle/asf.header.incremental` from a target of Apache RAT.
That's because the following content can not have the ASL header. We don't need
to change [the root
./pom.xml](https://github.com/apache/hive/blob/ad55d58eadd6c1aabc7fed51e25dfffe158a44f6/pom.xml#L1907)
because it is originally excluded.
```
^\/\*$
^( \*| \* +.*)$
^ \*\/$
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
