Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
gnodet merged 6 commits into from
Nov 14, 2022
Merged

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

gnodet merged 6 commits into from
Nov 14, 2022

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented Oct 22, 2022 •
edited by adangel
Loading

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)

@gnodet gnodet mentioned this pull request Oct 22, 2022
Closed
@gnodet gnodet requested a review from olamy October 22, 2022 10:41
@gnodet gnodet mentioned this pull request Oct 22, 2022
Closed
@gnodet gnodet added this to the MAVEN-38 milestone Oct 22, 2022
@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'

bmarwell
bmarwell approved these changes Oct 23, 2022
View reviewed changes
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

olamy
olamy requested changes Oct 23, 2022
View reviewed changes
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.

see #82 (comment)

@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 •
edited
Loading

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.

slawekjaranowski
slawekjaranowski reviewed Oct 23, 2022
View reviewed changes
pom.xml Outdated
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-shared-resources</artifactId>
<version>5-SNAPSHOT</version>
Copy link
Member

@slawekjaranowski slawekjaranowski Oct 23, 2022

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

@gnodet gnodet Oct 23, 2022

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

@gnodet gnodet Oct 24, 2022

Choose a reason for hiding this comment

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

apache/maven-shared-resources#5

@gnodet gnodet merged commit ea48d2a into master Nov 14, 2022
@slawekjaranowski slawekjaranowski deleted the MPOM-349-spotless branch April 13, 2024 07:51
@jira-importer
Copy link

jira-importer commented Mar 6, 2025

Resolve #275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdemers bdemers bdemers left review comments

@bmarwell bmarwell bmarwell approved these changes

@slawekjaranowski slawekjaranowski slawekjaranowski left review comments

@olamy olamy olamy approved these changes

@slachiewicz slachiewicz slachiewicz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

MAVEN-38

Development

Successfully merging this pull request may close these issues.

7 participants

@gnodet @olamy @jira-importer @bdemers @bmarwell @slawekjaranowski @slachiewicz