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

Fix checkstyle plugin, fix errors reported by checkstyle #234

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

deining
Copy link

@deining deining commented Apr 2, 2021

From a previous discussion, I was invited to fix checkstyle currently not working properly. This PR is my take on that. What it does:

  • pom.xl:
    • make checkstyle plugin work again
    • fix warning about required maven version
  • fixes 13 errors reported by checkstyle plugin
  • adds a maven wrapper to the project.

TODO: run checkstyle in CI, this is left up for you.

Copy link
Member

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

Thanks... and sorry :( I appreciate the effort here, but:

  • For formatting, we generally rely on http://github.com/google/google-java-format. It looks like this Checkstyle configuration wants some changes that would run counter to that. Checkstyle does also request some changes that we'd want. But we would probably want to either reformat the whole project or else reformat sections only as we touch them. (And I'm not sure which we'd want.)

  • For Maven Wrapper: That would be an improvement. However, for technical reasons, our internal build would require probably most of a day to set up with Maven Wrapper. That's doable, but it's hard to justify doing over lots of other things. Now, most of the cost for Maven Wrapper would be one-time setup. So, if someone does that setup someday, then we could pretty easily use it for Compile-Testing.

I have no reservations about the https changes and the changes in JavaFileObjects and MoreTrees. So if you want to have a PR that covers just those, we should be able to take that quickly.

@cpovirk cpovirk added P3 type=other Miscellaneous activities not covered by other type= labels labels Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes P3 type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants