Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MPOM-349] Apply spotless to check/reformat code + poms #82

Merged
merged 6 commits into from
Nov 14, 2022

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Oct 22, 2022

JIRA issue: https://issues.apache.org/jira/browse/MPOM-349

  • [MPOM-349] Apply spotless to check/reformat code + poms
  • [MPOM-349] Reformat poms

Depends on apache/maven-shared-resources#5

Discussed on

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)

@olamy
Copy link
Member

olamy commented Oct 23, 2022

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'

Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits, otherwise+1

pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet
Copy link
Contributor Author

gnodet commented Oct 23, 2022

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.

@olamy
Copy link
Member

olamy commented Oct 23, 2022

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.

@gnodet
Copy link
Contributor Author

gnodet commented Oct 23, 2022

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 ?

@gnodet
Copy link
Contributor Author

gnodet commented Oct 23, 2022

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.

pom.xml Outdated
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-shared-resources</artifactId>
<version>5-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use release version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet gnodet merged commit ea48d2a into master Nov 14, 2022
@slawekjaranowski slawekjaranowski deleted the MPOM-349-spotless branch April 13, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants