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 config file #4706

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
6 participants
@chfast
Collaborator

chfast commented Dec 14, 2017

No description provided.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 14, 2017

Codecov Report

Merging #4706 into develop will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4706      +/-   ##
===========================================
+ Coverage    60.92%   60.92%   +<.01%     
===========================================
  Files          343      343              
  Lines        27089    27089              
  Branches      2838     2838              
===========================================
+ Hits         16503    16504       +1     
- Misses        9589     9593       +4     
+ Partials       997      992       -5

codecov-io commented Dec 14, 2017

Codecov Report

Merging #4706 into develop will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4706      +/-   ##
===========================================
+ Coverage    60.92%   60.92%   +<.01%     
===========================================
  Files          343      343              
  Lines        27089    27089              
  Branches      2838     2838              
===========================================
+ Hits         16503    16504       +1     
- Misses        9589     9593       +4     
+ Partials       997      992       -5

@chfast chfast requested review from pirapira and gumb0 Dec 18, 2017

@chfast chfast referenced this pull request Dec 18, 2017

Closed

Clang format #4670

@pirapira

Looks good to me.

unless they are labeled with `[in progress]` or have "[WIP]" in title.
2. **Code formatting** rules are described by the [Clang-Format Style Options] file [.clang-format].
Please use the [clang-format] tool to format your code _changes_ accordingly.

This comment has been minimized.

@gumb0

gumb0 Dec 18, 2017

Member

Maybe add the exact command to do this?

@gumb0

gumb0 Dec 18, 2017

Member

Maybe add the exact command to do this?

This comment has been minimized.

@chfast

chfast Dec 18, 2017

Collaborator

The example below is ok?

@chfast

chfast Dec 18, 2017

Collaborator

The example below is ok?

This comment has been minimized.

@JoeLoser

JoeLoser Dec 19, 2017

I think the example below is fine. Perhaps we should create a commit hook to apply clang-format on the changed cpp files? We can install the hook at CMake time.

@JoeLoser

JoeLoser Dec 19, 2017

I think the example below is fine. Perhaps we should create a commit hook to apply clang-format on the changed cpp files? We can install the hook at CMake time.

This comment has been minimized.

@chfast

chfast Dec 19, 2017

Collaborator

Do you mean this hook? https://github.com/andrewseidl/githook-clang-format
Both the hook and installation with CMake seems quite invasive. Let's see what happens without it first.

@chfast

chfast Dec 19, 2017

Collaborator

Do you mean this hook? https://github.com/andrewseidl/githook-clang-format
Both the hook and installation with CMake seems quite invasive. Let's see what happens without it first.

This comment has been minimized.

@JoeLoser

JoeLoser Dec 19, 2017

That particular one works, yep. While invasive certainly, it forces clang-format to always be run. Otherwise, if a PR gets merged without adhering to the style specified in .clang-format, then the next person's commit that touches one of those files and runs clang-format will see diffs on lines they did not originally touch, due to the previous person not running clang-format. So, having it run automatically for everyone eliminates this issue.

But, I do hear you that perhaps this is premature to worry about this before we have the issue of someone not running clang-format on their changed files.

@JoeLoser

JoeLoser Dec 19, 2017

That particular one works, yep. While invasive certainly, it forces clang-format to always be run. Otherwise, if a PR gets merged without adhering to the style specified in .clang-format, then the next person's commit that touches one of those files and runs clang-format will see diffs on lines they did not originally touch, due to the previous person not running clang-format. So, having it run automatically for everyone eliminates this issue.

But, I do hear you that perhaps this is premature to worry about this before we have the issue of someone not running clang-format on their changed files.

This comment has been minimized.

@chfast

chfast Dec 20, 2017

Collaborator

It's hard to force automation on developers' side.

Let us figure out the process by practice. We have just found the git-clang-format tool. It seems to work great for our needs. We will document the workflow in CONTRIBUTING.md soon.

@chfast

chfast Dec 20, 2017

Collaborator

It's hard to force automation on developers' side.

Let us figure out the process by practice. We have just found the git-clang-format tool. It seems to work great for our needs. We will document the workflow in CONTRIBUTING.md soon.

@gumb0

gumb0 approved these changes Dec 18, 2017

@chfast chfast merged commit 983ad58 into develop Dec 19, 2017

8 of 9 checks passed

codecov/project/tests 78.84% (-0.05%) compared to 0bfab57
Details
ci/circleci: Linux-Clang5 Your tests passed on CircleCI!
Details
ci/circleci: Linux-GCC6-Debug Your tests passed on CircleCI!
Details
ci/circleci: macOS-XCode9 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 0bfab57...d54bcbb
Details
codecov/project 60.92% (+<.01%) compared to 0bfab57
Details
codecov/project/app 52.94% (+0.02%) compared to 0bfab57
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chfast chfast deleted the clang-format-config branch Dec 19, 2017

This was referenced Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment