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 --clang-format-version option #282

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Jan 4, 2021

Signed-off-by: Tyler Weaver tylerjw@gmail.com

Description

I am using clang-format-10. This change would enable me to use ament_clang_format with clang-format-10. Note that find_executable will return as soon as it finds a valid executable. This means that if we search in this order searching for clang-format- first causes no harm in the case where the user does not provide a value for this.

Follow-on work (for other PRs)

  • ament_cmake_clang_format should be updated to enable using this option from cmake.
  • Would it be a good practice to search the filesystem for a .clang_format file in the working directory or in a parent and use that instead if it exists for the config? This would simplify running ament_clang_format in a repo that contains a config. If this default behavior would be accepted, it should also be done for clang-tidy.

@tylerjw tylerjw force-pushed the add_clang-format-version_option branch from 5a1c980 to d747180 Compare January 18, 2021 18:56
@mm318
Copy link
Member

mm318 commented Jan 19, 2021

Can you also please update the doc regarding the new option that you're adding?

@mm318 mm318 self-assigned this Jan 19, 2021
@tylerjw tylerjw force-pushed the add_clang-format-version_option branch from d747180 to cdb4c9c Compare January 19, 2021 16:22
Signed-off-by: Tyler Weaver <tylerjw@gmail.com>
@tylerjw tylerjw force-pushed the add_clang-format-version_option branch from cdb4c9c to 61c84f7 Compare January 19, 2021 16:23
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 19, 2021

I updated the documentation to include instructions for setting these variables. I will create a follow-on PR for ament_cmake_clang_format to allow setting this option from the cmake function later.

Copy link
Member

@mm318 mm318 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, pending CI (I will start CI as soon as I can).

@mm318
Copy link
Member

mm318 commented Jan 20, 2021

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mm318 mm318 merged commit ecf1cce into ament:master Jan 20, 2021
@tylerjw tylerjw deleted the add_clang-format-version_option branch January 20, 2021 22:18
@mikepurvis
Copy link

mikepurvis commented Jun 28, 2023

I updated the documentation to include instructions for setting these variables. I will create a follow-on PR for ament_cmake_clang_format to allow setting this option from the cmake function later.

AFAICT this didn't end up happening?

Edit: Ah, I see from #289 (comment) that you ended up solving the issue a different way.

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.

None yet

3 participants