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

Add and apply Checkstyle config. #389

Merged
merged 30 commits into from
Sep 13, 2021
Merged

Conversation

blackwinter
Copy link
Member

@blackwinter blackwinter commented Sep 5, 2021

Based on https://github.com/metafacture/metafacture-fix/blob/master/config/checkstyle/checkstyle.xml (metafacture/metafacture-fix#11).

We need those (annoying) checks from metafacture-fix here :-)

Originally posted by @fsteeg in #387 (comment)

This would be part 2 (see #388 for part 1).

Notes:

  • Checks have been disabled for test sources; maybe they should be enabled at a later stage?
  • Suppressed rules (checkstyle-disable-line) should be evaluated; maybe adjust the Checkstyle config.
  • Most obvious nuisances: MagicNumber, MissingCtor, ReturnCount
  • metafacture/metafacture-fix@d70d5d3 introduced suppression via annotations in addition to comments; maybe we should stick to one method.

@blackwinter blackwinter requested a review from fsteeg September 5, 2021 15:26
@blackwinter blackwinter self-assigned this Sep 5, 2021
Base automatically changed from updateAndApplyEditorConfig to master September 6, 2021 08:46
@blackwinter blackwinter force-pushed the addAndApplyCheckstyleConfig branch from 9493ebf to 52c9559 Compare September 6, 2021 08:47
@blackwinter blackwinter requested a review from dr0i September 9, 2021 20:08
@blackwinter blackwinter marked this pull request as ready for review September 9, 2021 20:08
@blackwinter blackwinter assigned fsteeg and dr0i and unassigned blackwinter Sep 9, 2021
@blackwinter
Copy link
Member Author

Oh well, looks like I've touched virtually every single Java file there is: 496/499 🤯

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

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

Uh, this is a lot of change ... hard to check everything. After my scanning I think it's good.

Checks have been disabled for test sources ...

Very good - so it's clear with one glimpse that the code (style) changes didn't change the outcome of the tests (there are some tests changed - but it's just adding a newline before package declaration).

... ; maybe they should be enabled at a later stage?

+1

@dr0i dr0i removed their assignment Sep 10, 2021
@blackwinter
Copy link
Member Author

Uh, this is a lot of change ... hard to check everything.

Yes, sorry about that! But if it's any consolation, it was also a lot to write ;)

After my scanning I think it's good.

Thanks!

there are some tests changed - but it's just adding a newline before package declaration

I only applied very few automatic changes to the test sources.

@blackwinter blackwinter changed the title Add and apply checkstyle config Add and apply Checkstyle config. Sep 10, 2021
Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

Phew, awesome, many thanks for setting this up! 🎉

Oddly, I had to run ./gradlew editorconfigFormat locally once (but resulting in no changes on git diff) to get rid of violation errors (whitespace/newline issues).

@fsteeg fsteeg assigned blackwinter and unassigned fsteeg Sep 13, 2021
@blackwinter
Copy link
Member Author

Thanks for the review!

Oddly, I had to run ./gradlew editorconfigFormat locally once (but resulting in no changes on git diff) to get rid of violation errors (whitespace/newline issues).

So you had local (unversioned) files that were checked? Would you want those to get excluded/ignored?

@blackwinter blackwinter merged commit 511b4af into master Sep 13, 2021
@blackwinter blackwinter deleted the addAndApplyCheckstyleConfig branch September 13, 2021 15:25
@fsteeg
Copy link
Member

fsteeg commented Sep 14, 2021

So you had local (unversioned) files that were checked? Would you want those to get excluded/ignored?

No, no unversioned files. Maybe related to line endings managed by git internally (core.autocrlf)? No problem though, mostly a note in case anyone has the same issue locally.

@blackwinter blackwinter mentioned this pull request Sep 30, 2021
@blackwinter blackwinter mentioned this pull request Nov 2, 2021
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.

3 participants