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

Automate code formatting using Maven #2494

Closed

Conversation

walles
Copy link
Contributor

@walles walles commented Apr 2, 2021

Before this change

CONTRIBUTING.md encouraged reformatting sources using make format.

make format would reformat lots of files, not just the ones you changed.

With this change in place

Any mvn operation will implicitly format sources.

CI will validate code formatting.

This implicitly means that people have to at least mvn compile their sources locally before making PRs to make sure they don't get refused by CI.

This enforces good practice, regardless of the actual formatting.

Notes

Files to be reviewed (the rest have been auto formatted):

  • pom.xml
  • Makefile
  • .github/CONTRIBUTING.md

Also, checking the commits list might be easier than looking at files changed, all reformatting is confined to the last change.

The code formatting Maven plugin being used didn't have any validate goal so I replaced with another one.

Still running the Eclipse formatter though, so hbase-formatter.xml still works.

hbase-formatter.xml is from 2014. It could potentially be replaced with a newer one from 2019, I haven't done that though.

@walles walles marked this pull request as ready for review April 2, 2021 06:36
@sazzad16
Copy link
Contributor

sazzad16 commented Apr 2, 2021

@walles Would you be able to craft a separate PR with 2019 formatter? This would help us to do at least a visual comparison. Thanks either ways!

@walles
Copy link
Contributor Author

walles commented Apr 2, 2021

@sazzad16 I opened #2495 as well, same thing but with the updated formatting rules.

Personally I'd prefer using the newer formatting rule set I think, but take your pick!

@sazzad16
Copy link
Contributor

sazzad16 commented Apr 2, 2021

Thank you very much @walles

@walles
Copy link
Contributor Author

walles commented Aug 15, 2021

I think having one of these PRs open should be enough. Closing in favour of #2495.

@walles walles closed this Aug 15, 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.

2 participants