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

Clang format changes #429

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Clang format changes #429

merged 1 commit into from
Jul 12, 2024

Conversation

bsutherland333
Copy link
Contributor

After reviewing the ROS2 and Google style guides, @avtoku and myself realized that the .clang-format file was not completely in line with their standards. This updates the .clang-format file and the code to fix that.

@bsutherland333 bsutherland333 requested a review from avtoku July 10, 2024 23:21
@bsutherland333 bsutherland333 self-assigned this Jul 10, 2024
@avtoku
Copy link
Contributor

avtoku commented Jul 11, 2024

It looks like ROS2 has instructions for linting your code that go beyond a clang-format call.
Personally I'd be inclined to just leave it as the stock google .clang-format if we are not going to go all the way.

@bsutherland333
Copy link
Contributor Author

Ok, how about for now we just worry about the .clang-format file. Are you suggesting we leave it as is?

@avtoku
Copy link
Contributor

avtoku commented Jul 11, 2024

Ok, how about for now we just worry about the .clang-format file. Are you suggesting we leave it as is?

For Code that is compiled and run in the ROS2 environment, ROS2 recommends using their ament linter: https://docs.ros.org/en/humble/The-ROS2-Project/Contributing/Code-Style-Language-Versions.html#linters
This is beyond what I personally want to deal with for the rosflight_firmware.

For rosflight_firmware that lives outside the ROS2 environment we should not have to comply with ROS2 conventions. I think we should leave it as is for now. Consider changing:
AllowShortIfStatementsOnASingleLine: Always
to
AllowShortIfStatementsOnASingleLine: AllIfsAndElse

@bsutherland333
Copy link
Contributor Author

@avtoku How does that look?

@avtoku
Copy link
Contributor

avtoku commented Jul 12, 2024

@avtoku How does that look?

Works for me.

Copy link
Contributor

@avtoku avtoku left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@bsutherland333 bsutherland333 merged commit 8831f7b into main Jul 12, 2024
2 checks passed
@bsutherland333 bsutherland333 deleted the clang_format_changes branch July 12, 2024 15:26
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