laurentgo commented on code in PR #41825:
URL: https://github.com/apache/arrow/pull/41825#discussion_r1620188457
##########
java/algorithm/pom.xml:
##########
@@ -48,5 +48,49 @@
</dependency>
</dependencies>
- <build></build>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <configuration>
+ <formats>
+ <format>
+ <!-- configure license for xml files -->
+ <includes>
+ <include>pom.xml</include>
+ </includes>
+ <licenseHeader>
+ <file>${project.basedir}/../dev/license/asf-xml.license</file>
+ <delimiter>(<configuration|<project)</delimiter>
+ </licenseHeader>
+ </format>
+ <format>
+ <!-- configure license for java files -->
+ <includes>
Review Comment:
This `<format>` section has no step so I don't think there's a reason to
keep it?
##########
java/algorithm/pom.xml:
##########
@@ -48,5 +48,49 @@
</dependency>
</dependencies>
- <build></build>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <configuration>
+ <formats>
+ <format>
Review Comment:
assuming we keep license formatting for pom.xml only for this module, we can
still merge with the parent pom.xml by adding `combine.children="append"`
attribute to the `<pom>` element (and remove the need to specify the list of
files to include as those are automatically deduced from the section)
##########
java/algorithm/pom.xml:
##########
@@ -48,5 +52,87 @@
</dependency>
</dependencies>
- <build></build>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>3.1.0</version>
+ <configuration>
+ <skip>true</skip>
Review Comment:
What about creating a build property in parent pom
`checkstyle.config.location` defaulting to `dev/checkstyle/checkstyle.xml` and
in `algorithm` module changing it to `dev/checkstyle/checkstyle-spotless.xml`?
The new file could be a copy of the original minus the conflicting checkers.
I would suggest for the initial revision to comment the checkers instead of
removing them to make it easier to review and confirm those are indeed
conflicting checkers vs just a config change.
##########
java/algorithm/pom.xml:
##########
@@ -48,5 +52,87 @@
</dependency>
</dependencies>
- <build></build>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>3.1.0</version>
+ <configuration>
+ <skip>true</skip>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <profiles>
+ <profile>
+ <id>spotless</id>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>${spotless.version}</version>
+ <configuration>
+ <formats>
+ <format>
Review Comment:
The configuration for pom could be moved to the parent with path being
`${maven.multiModuleProjectDirectory}/java/dev/license/asf-xml.license}`
(should be safe to use because the project has a `.mvn` directory present). As
it will reformat several existing `pom.xml` who inherits the configuration
(there's a small indentation change), maybe we should address it as a
standalone change like we did for #41173
As for the java format, yes it would be ideal to move to the parent as well
but there's no good mechanism yet to limit the amount of change. The best I can
think of is to do this in the parent pom
```
<properties>
<!-- replace with empty string to enable java formatting in submodules
-->
<spotless.java.excludes>**</spotless.java.excludes>
</properties>
<build>
<plugins>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<configuration>
<java>
<excludes>
<exclude>${spotless.java.excludes}</exclude>
</excludes>
<googleJavaFormat>
<version>1.7</version>
<style>GOOGLE</style>
</googleJavaFormat>
<licenseHeader>
<file>${maven.multiModuleProjectDirectoryjava/dev/license/asf-java.license</file>
<delimiter>package</delimiter>
</licenseHeader>
</java>
[...]
</configuration>
[...]
</plugin>
</plugins>
</build>
```
This would limit the amount of changes to the other pom.xml files and the
final cleanup to be done by a large amount
--
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]
