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

Issue126 clang format license check #156

Merged

Conversation

Alan-Jowett
Copy link
Collaborator

@Alan-Jowett Alan-Jowett commented Oct 13, 2022

Add Contributing.md guide to propose / document style guidelines.
Add scripts to check formatting and licensing at checkin.

Resolves: #126

Signed-off-by: Alan Jowett alanjo@microsoft.com

@coveralls
Copy link

coveralls commented Oct 13, 2022

Coverage Status

Coverage decreased (-2.1%) to 81.506% when pulling fde192f on Alan-Jowett:issue126-clang-format-license-check into 049f603 on iovisor:main.

Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

I would like to see a .editorconfig to go with the .clang-format file as that helps editors implement the 120 character column limit, and indentation rules (and new-lines at end of files).

There are some other technical details (see comments on bits of source) that I think need working out but I'm overall happy with the direction taken.

scripts/pre-commit Outdated Show resolved Hide resolved
docs/Contributing.md Outdated Show resolved Hide resolved
docs/Contributing.md Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

Thank you for the update.

One or two changes in the editorconfig suggested. Of which only one is a real issue in my opinion.

.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@Alan-Jowett Alan-Jowett force-pushed the issue126-clang-format-license-check branch from 25f8af1 to 86860e6 Compare October 18, 2022 17:04
Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

Some small changes. Sorry - this is the type of PR where something minor is found each time. But it is getting there.

cmake/options.cmake Outdated Show resolved Hide resolved
docs/Contributing.md Outdated Show resolved Hide resolved
vm/ubpf_jit_arm64.c Show resolved Hide resolved
Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

Happy with this now. @Alan-Jowett: Do you mind doing the conflict resolution in the merge?

@Alan-Jowett Alan-Jowett force-pushed the issue126-clang-format-license-check branch from 67adb66 to 69cdb72 Compare October 20, 2022 16:08
Add docs/Contributing.md
Add .editorconfig
Apply clang formatting to existing files

Signed-off-by: Alan Jowett <alanjo@microsoft.com>
@Alan-Jowett Alan-Jowett force-pushed the issue126-clang-format-license-check branch from 69cdb72 to fde192f Compare October 20, 2022 16:18
@Alan-Jowett Alan-Jowett merged commit 5cd99a6 into iovisor:main Oct 20, 2022
@Alan-Jowett Alan-Jowett deleted the issue126-clang-format-license-check branch October 20, 2022 16:33
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.

Adopt clang formatting for repo
3 participants