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 pre-commit for automatic formatting #248

Closed
wants to merge 1 commit into from

Conversation

pac48
Copy link
Contributor

@pac48 pac48 commented Oct 29, 2022

This PR adds a pre-commit config to run clang formatting. I checked the CONTRIBUTING.md and didn't see any format requirement. For anyone not familiar with pre-commit, here is a helpful tutorial: Check formatting locally and on GitHub Actions with pre-commit

In short, pre-commit can be installed via: pip install pre-commit
and ran using: pre-commit run --all

The formatting is setup to run as part of the CI.

Signed-off-by: Paul Gesel <[email protected]>
@fmrico
Copy link
Contributor

fmrico commented Nov 1, 2022

Hi @pac48

I need a little more context in this PR. What are the benefits of using clang formatting instead of the ROS 2 standard?

Best
Francisco

@pac48
Copy link
Contributor Author

pac48 commented Nov 1, 2022

Hi @pac48

I need a little more context in this PR. What are the benefits of using clang formatting instead of the ROS 2 standard?

Best Francisco

Hi @fmrico

Sorry for not elaborating, let me explain my issue. I opened a PR Add Support for Contingency Planning, which has a huge diff because I used a different format. That is no problem, the code can be reformatted, so I thought adding clang automatic formatting as part of CI would be nice. The are already popular ROS repos that have switched to using clang format, MoveIt 2, MoveIt BehaviorTree.CPP.

One advantage of the clang-format over the ament based tools is it is the de facto standard in C++. It is a consistent and powerful tool for formatting. The real downside to clang-format is that it is kind of annoying to run locally. But that is where pre-commit comes in. It is a python script that automates running tools like clang-format locally for developers and can also be used in CI to verify the formatting stays consistent. With pre-commit, you can also verify the formatting of python files, spell check, check YAML formatting, etc. The list goes on. When applying to the this repo, there were actually quite a few changes recommended by the python formater "black". There were also many spelling mistakes that were caught and suggestions were recommended.

The biggest problem with clang-format is that it can't be applied to some existing ROS 2 code bases because there are some small formatting exceptions it does not allow. See here for a discussion.

I don't have a strong opinion on which formater should be used, although it should be within the ROS code style guidelines of course. With that said, here is a branch from master after running ament_uncrustify --reformat
run ament reformat. You can see there is a huge diff still. I think having some automatic formatting will save headaches in the future.

Is there some other tool (besides clang-format and ament_uncrustify) that you recommend?

@fmrico
Copy link
Contributor

fmrico commented Nov 8, 2022

I have not forgotten this PR. I need time to think deeply about this.

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