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

Bring clang-format in line with developer guide #147

Merged
merged 5 commits into from
Aug 6, 2019

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Jul 3, 2019

Previously, ament_clang_format's defaults were very divergent from ament_uncrustify. Now the default config file has been rewritten to match the developer guide.
https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#id26

@dirk-thomas
Copy link
Contributor

Please keep the list of option in alphabetical order. That will hopefully also improve the readability of the diff.

Regarding the deleted entries: did you delete them because they are the same as in the Google style? Not sure if it would make sense to keep them explicitly for easier review. If not, I think having two separate commits - one for changing values and one for removing values which are the same as the default - would be good.

Signed-off-by: Dan Rose <[email protected]>
@rotu
Copy link
Contributor Author

rotu commented Jul 15, 2019

Thanks for the feedback. I had them in the order they were mentioned in the dev guide, but alphabetical makes more sense.

Yes, I deleted extra entries because they are the same as in the Google style, but also because the ROS2 dev guide didn't mention them, and I wanted this to be as faithful as possible to that document.

@dirk-thomas
Copy link
Contributor

Have you tried to use clang format with the updated configuration on the ROS 2 code base? Currently clang format is excluded by default (see

<!--<exec_depend>ament_clang_format</exec_depend>-->
). In order for CI to use it it needs to be enabled.

@rotu
Copy link
Contributor Author

rotu commented Jul 17, 2019

Yes, I use clang-format with that config on my code. No, I can't fully use it on the ROS2 code base. It still cannot be enabled by default since it does not agree with ament_uncrustify in all cases.

@rotu
Copy link
Contributor Author

rotu commented Aug 6, 2019

@dirk-thomas What's the status of this PR? Are there changes you're waiting on?

@dirk-thomas
Copy link
Contributor

Since this isn't used anywhere I am happy to merge it as is.

@dirk-thomas dirk-thomas merged commit 14178ab into ament:master Aug 6, 2019
@rotu rotu deleted the master branch August 6, 2019 19:42
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