laurentgo commented on code in PR #41825:
URL: https://github.com/apache/arrow/pull/41825#discussion_r1616308609
##########
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>
Review Comment:
`<activeByDefault>` is actually automatically disabled when another profile
is used. Instead it should be part of the main `<build>` section. If developers
need to disable it, it is possible to use `-Dspotless.skip`.
##########
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:
Note that all checkstyle configuration options present in Apache Arrow are
not Java style issues and would not be detected by `spotless/google-java-format`
##########
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>
+ <!-- configure license for xml files -->
+ <includes>
+ <include>pom.xml</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-xml.license</file>
+ <delimiter>(<configuration|<project)</delimiter>
+ </licenseHeader>
+ </format>
+ <format>
+ <!-- configure license for java files -->
+ <includes>
+ <include>**/*.java</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-java.license</file>
+ <delimiter>package</delimiter>
+ </licenseHeader>
+ </format>
+ </formats>
+ <java>
+ <googleJavaFormat>
+ <version>1.17.0</version>
+ <style>GOOGLE</style>
+ </googleJavaFormat>
+ </java>
+ <pom>
+ <indent>
+ <tabs>true</tabs>
+ <spacesPerTab>2</spacesPerTab>
+ </indent>
+ <indent>
+ <spaces>true</spaces>
+ <spacesPerTab>2</spacesPerTab>
+ </indent>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
+ </sortPom>
+ </pom>
+ </configuration>
+ <executions>
Review Comment:
This execution is already defined
[here](https://github.com/apache/arrow/blob/ff9921ffa89585be69ae85674bb365d03cb22ba4/java/pom.xml#L840)
##########
java/algorithm/pom.xml:
##########
@@ -20,6 +20,10 @@
<name>Arrow Algorithms</name>
<description>(Experimental/Contrib) A collection of algorithms for working
with ValueVectors.</description>
+ <properties>
+ <spotless.version>2.30.0</spotless.version>
Review Comment:
Already defined in `java/pom.xml`
##########
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>
Review Comment:
Version definition is already defined
[here](https://github.com/apache/arrow/blob/ff9921ffa89585be69ae85674bb365d03cb22ba4/java/pom.xml#L523)
##########
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>
Review Comment:
it is not necessary to add the version here as it is already defined by the
parent module (or its parent)
##########
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>
+ <!-- configure license for xml files -->
+ <includes>
+ <include>pom.xml</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-xml.license</file>
+ <delimiter>(<configuration|<project)</delimiter>
+ </licenseHeader>
+ </format>
+ <format>
+ <!-- configure license for java files -->
+ <includes>
+ <include>**/*.java</include>
+ </includes>
+ <licenseHeader>
Review Comment:
This can be moved under the `java` section according to
[here](https://github.com/diffplug/spotless/tree/main/plugin-maven#java)
##########
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>
+ <!-- configure license for xml files -->
+ <includes>
+ <include>pom.xml</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-xml.license</file>
+ <delimiter>(<configuration|<project)</delimiter>
+ </licenseHeader>
+ </format>
+ <format>
+ <!-- configure license for java files -->
+ <includes>
+ <include>**/*.java</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-java.license</file>
+ <delimiter>package</delimiter>
+ </licenseHeader>
+ </format>
+ </formats>
+ <java>
+ <googleJavaFormat>
+ <version>1.17.0</version>
+ <style>GOOGLE</style>
+ </googleJavaFormat>
+ </java>
+ <pom>
+ <indent>
+ <tabs>true</tabs>
+ <spacesPerTab>2</spacesPerTab>
+ </indent>
+ <indent>
+ <spaces>true</spaces>
+ <spacesPerTab>2</spacesPerTab>
+ </indent>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
+ </sortPom>
+ </pom>
+ </configuration>
+ <executions>
+ <execution>
+ <id>spotless-check</id>
+ <goals>
+ <goal>apply</goal>
Review Comment:
`apply` goal is not bound to any phase so there's no need to add it to an
execution, unless we would want apply to run during build (but it would cause
some issues with CI)
##########
java/spotless/asf-java.license:
##########
Review Comment:
This file is the same as `java/dev/checkstyle/checkstyle.license`. Maybe we
can rename it as `java/dev/license/asf-java.license` and use it for both
plugins?
##########
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:
This can be combined with the `pom` section below
##########
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>
+ <!-- configure license for xml files -->
+ <includes>
+ <include>pom.xml</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-xml.license</file>
+ <delimiter>(<configuration|<project)</delimiter>
+ </licenseHeader>
+ </format>
+ <format>
+ <!-- configure license for java files -->
+ <includes>
+ <include>**/*.java</include>
+ </includes>
+ <licenseHeader>
+ <file>../spotless/asf-java.license</file>
+ <delimiter>package</delimiter>
+ </licenseHeader>
+ </format>
+ </formats>
+ <java>
+ <googleJavaFormat>
+ <version>1.17.0</version>
+ <style>GOOGLE</style>
+ </googleJavaFormat>
+ </java>
+ <pom>
Review Comment:
There's already a configuration for `pom` at the top level so it may not be
necessary to redefine it. Also adding indent actions before sortPom seems
unnecessary as sortPom will reindent the files anyway?
--
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]
