You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/10/22 09:50:56 UTC
[GitHub] [maven-parent] gnodet opened a new pull request, #82: [MPOM-349] Apply spotless to check/reformat code + poms
gnodet opened a new pull request, #82:
URL: https://github.com/apache/maven-parent/pull/82
JIRA issue: https://issues.apache.org/jira/browse/MPOM-349
- [MPOM-349] Apply spotless to check/reformat code + poms
- [MPOM-349] Reformat poms
Discussed on
* https://lists.apache.org/thread/ok9qdl1w92f9fhdkz3tmc5f65ytpppqpD
* https://lists.apache.org/thread/md6do2dllt9cv4s311y3ky6245xmvd0w
The PR includes the following changes:
* add spotless with palantir formatter (120 column, 4 spaces indent) + pom formatter (2 spaces indent + pom ordering)
* disable checkstyle rules enforced by the formatter
* enable formatter check (code style validation) by default
* provide a format profile (activated with the format property) to apply formatting rules automatically
* reformat poms to pass the new checks (in a subsequent commit to ease review)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002675689
##########
pom.xml:
##########
@@ -104,10 +102,10 @@ under the License.
<id>cstamas</id>
<name>Tamas Cservenak</name>
<email>cstamas@apache.org</email>
- <timezone>+1</timezone>
<roles>
<role>PMC Member</role>
</roles>
+ <timezone>+1</timezone>
Review Comment:
Same here, I'd rather have this PR focus on just the spotless addition. The reformat commit should not change anyy content, and I would keep both commits for reference.
So I'd rather not change anything unrelated to the formatting stuff.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] bdemers commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
bdemers commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002661857
##########
pom.xml:
##########
@@ -294,20 +292,20 @@ under the License.
<id>adangel</id>
<name>Andreas Dangel</name>
<email>adangel@apache.org</email>
- <timezone>Europe/Berlin</timezone>
<roles>
<role>Committer</role>
</roles>
+ <timezone>Europe/Berlin</timezone>
</developer>
<developer>
<id>bdemers</id>
<name>Brian Demers</name>
- <organization>Sonatype</organization>
<email>bdemers@apache.org</email>
- <timezone>-5</timezone>
+ <organization>Sonatype</organization>
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] bmarwell commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002660686
##########
pom.xml:
##########
@@ -294,20 +292,20 @@ under the License.
<id>adangel</id>
<name>Andreas Dangel</name>
<email>adangel@apache.org</email>
- <timezone>Europe/Berlin</timezone>
<roles>
<role>Committer</role>
</roles>
+ <timezone>Europe/Berlin</timezone>
</developer>
<developer>
<id>bdemers</id>
<name>Brian Demers</name>
- <organization>Sonatype</organization>
<email>bdemers@apache.org</email>
- <timezone>-5</timezone>
+ <organization>Sonatype</organization>
Review Comment:
That's outdated. You might want to update or remove it while at it. He's now working for Okta.
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
Review Comment:
Expanding empty elements would allow less diff noise.
##########
pom.xml:
##########
@@ -104,10 +102,10 @@ under the License.
<id>cstamas</id>
<name>Tamas Cservenak</name>
<email>cstamas@apache.org</email>
- <timezone>+1</timezone>
<roles>
<role>PMC Member</role>
</roles>
+ <timezone>+1</timezone>
Review Comment:
This should be a timezone name, while at it. But works for me.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] olamy commented on pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
olamy commented on PR #82:
URL: https://github.com/apache/maven-parent/pull/82#issuecomment-1288023514
LGTM but if we move to spotless. I would remove `checkstyle:check` as well as we do not need really to check sources style twice during the build....
And it's a bit confusing as when testing this pom with m-war-p, `checkstyle:check` is passing but not `spotless:check'
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #82:
URL: https://github.com/apache/maven-parent/pull/82#issuecomment-1288069391
> LGTM but if we move to spotless. I would remove `checkstyle:check` as well as we do not need really to check sources style twice during the build....
> And it's a bit confusing as when testing this pom with m-war-p, `checkstyle:check` is passing but not `spotless:check'
I've disabled the checkstyle checks that could conflict with the ones checked by spotless, so what you see is expected. The remaining checkstyle checks should be related to non stylistic issues: naming, ordering, javadoc, method/file length, modifiers ordering, inner assignment, magic numbers, etc... So those checks can not really be fixed without refactoring the code. Makes more sense ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #82:
URL: https://github.com/apache/maven-parent/pull/82#issuecomment-1288064676
> LGTM but if we move to spotless. I would remove `checkstyle:check` as well as we do not need really to check sources style twice during the build....
> And it's a bit confusing as when testing this pom with m-war-p, `checkstyle:check` is passing but not `spotless:check'
@olamy Checkstyle checks method length, file length, qualifier order, parameter numbers, field/method ordering, etc... Those won't be checked by pure code style validation. I don't really see the value for removing those checks.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] slawekjaranowski commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002739612
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
+ </sortPom>
+ </pom>
+ <upToDateChecking>
+ <enabled>true</enabled>
+ </upToDateChecking>
+ </configuration>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.maven.shared</groupId>
+ <artifactId>maven-shared-resources</artifactId>
+ <version>5-SNAPSHOT</version>
Review Comment:
We should use release version.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002757523
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
+ </sortPom>
+ </pom>
+ <upToDateChecking>
+ <enabled>true</enabled>
+ </upToDateChecking>
+ </configuration>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.maven.shared</groupId>
+ <artifactId>maven-shared-resources</artifactId>
+ <version>5-SNAPSHOT</version>
Review Comment:
Argh, well spotted. I think it may be easier to put the removal of the related check styles in the checkstyle rules that `maven-shared-resources` provides then. I'll raise a PR there.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] bmarwell commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002744187
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
Review Comment:
Depends. If it doesn't expect child elements but just a string it's fine. If it expects child elements, the diff could have been shorter, but I really don't mind this setting. Ok for me.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002675205
##########
pom.xml:
##########
@@ -294,20 +292,20 @@ under the License.
<id>adangel</id>
<name>Andreas Dangel</name>
<email>adangel@apache.org</email>
- <timezone>Europe/Berlin</timezone>
<roles>
<role>Committer</role>
</roles>
+ <timezone>Europe/Berlin</timezone>
</developer>
<developer>
<id>bdemers</id>
<name>Brian Demers</name>
- <organization>Sonatype</organization>
<email>bdemers@apache.org</email>
- <timezone>-5</timezone>
+ <organization>Sonatype</organization>
Review Comment:
Well, that's completely out of scope of this PR...
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] olamy commented on pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
olamy commented on PR #82:
URL: https://github.com/apache/maven-parent/pull/82#issuecomment-1288066873
> > LGTM but if we move to spotless. I would remove `checkstyle:check` as well as we do not need really to check sources style twice during the build....
> > And it's a bit confusing as when testing this pom with m-war-p, `checkstyle:check` is passing but not `spotless:check'
>
> @olamy Checkstyle checks method length, file length, qualifier order, parameter numbers, field/method ordering, etc... Those won't be checked by pure code style validation. I don't really see the value for removing those checks.
ah right sorry.
but still everything concerning style in checkstyle (sorry but the name is confusing ;) ) can be removed.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002675539
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
Review Comment:
I don't see how. This just replaces `<foo />` with `<foo></foo>` or vice-versa. I do find the latter a bit weird to read. Not sure how that changes diffs. Can you elaborate ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #82:
URL: https://github.com/apache/maven-parent/pull/82#issuecomment-1288069866
> > > LGTM but if we move to spotless. I would remove `checkstyle:check` as well as we do not need really to check sources style twice during the build....
> > > And it's a bit confusing as when testing this pom with m-war-p, `checkstyle:check` is passing but not `spotless:check'
> >
> >
> > @olamy Checkstyle checks method length, file length, qualifier order, parameter numbers, field/method ordering, etc... Those won't be checked by pure code style validation. I don't really see the value for removing those checks.
>
> ah right sorry. but still everything concerning style in checkstyle (sorry but the name is confusing ;) ) can be removed. I mean such https://github.com/apache/maven-shared-resources/blob/f9f3527cd3546e208f5d13e1ca37ba763bac6d0c/src/main/resources/config/maven_checks.xml#L61
>
> nothing really urgent as long as it's in sync.
Yes, the problem is that I was fearing a bit removing those checks from `maven-shared-resources`. If a projet upgrades this dependency but does not use the latest parent, it will have some checks not enforced anymore. But yes, at some point, it should be synced.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet commented on a diff in pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #82:
URL: https://github.com/apache/maven-parent/pull/82#discussion_r1002922674
##########
pom.xml:
##########
@@ -1170,19 +1175,63 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>com.diffplug.spotless</groupId>
+ <artifactId>spotless-maven-plugin</artifactId>
+ <version>2.27.1</version>
+ <configuration>
+ <java>
+ <importOrder>
+ <file>config/maven-eclipse-importorder.txt</file>
+ </importOrder>
+ <removeUnusedImports/>
+ <palantirJavaFormat/>
+ <licenseHeader>
+ <file>config/maven-header-plain.txt</file>
+ </licenseHeader>
+ </java>
+ <pom>
+ <sortPom>
+ <expandEmptyElements>false</expandEmptyElements>
+ </sortPom>
+ </pom>
+ <upToDateChecking>
+ <enabled>true</enabled>
+ </upToDateChecking>
+ </configuration>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.maven.shared</groupId>
+ <artifactId>maven-shared-resources</artifactId>
+ <version>5-SNAPSHOT</version>
Review Comment:
https://github.com/apache/maven-shared-resources/pull/5
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-parent] gnodet merged pull request #82: [MPOM-349] Apply spotless to check/reformat code + poms
Posted by GitBox <gi...@apache.org>.
gnodet merged PR #82:
URL: https://github.com/apache/maven-parent/pull/82
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org