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 custom config file support for clang-format #153

Merged
merged 5 commits into from
Jul 23, 2019

Conversation

JShep1
Copy link
Contributor

@JShep1 JShep1 commented Jul 17, 2019

Currently only the .clang-format file in the configuration/ directory can be used. This update adds support for custom .clang-format files placed in the directory from which ament_clang_format is called.

Signed-off-by: John Shepherd johnshepherd96@yahoo.com

Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
@JShep1 JShep1 requested a review from dirk-thomas July 18, 2019 17:11
@dirk-thomas
Copy link
Contributor

Using a file in the cwd is kind of opaque. You have to know that this feature exists in order to use it.

I would suggest to instead add a command line option to the CLI as well as a keyword argument and global variable to the CMake API.

Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
@JShep1
Copy link
Contributor Author

JShep1 commented Jul 22, 2019

Added suggested CMake changes

Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
@JShep1 JShep1 requested a review from dirk-thomas July 23, 2019 23:20
@dirk-thomas dirk-thomas added the enhancement New feature or request label Jul 23, 2019
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
@JShep1 JShep1 merged commit 3ba6365 into master Jul 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the jshep1/add_clang_custom_config branch July 23, 2019 23:35
@wjwwood
Copy link
Contributor

wjwwood commented Jul 24, 2019

Looks like this introduced some linter failures on the nightly CI:

https://ci.ros2.org/view/nightly/job/nightly_linux_release/1247/testReport/(root)/projectroot/lint_cmake/

@JShep1 can you please fix those up? Also, it's preferable if you run CI even for small changes like this before merging. Thanks!

@wjwwood
Copy link
Contributor

wjwwood commented Jul 24, 2019

Btw, If you need help running the tests locally, or configuring CI so it's as quick as possible while still covering the change, let me know.

@JShep1
Copy link
Contributor Author

JShep1 commented Jul 25, 2019

Resolved in #162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants