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

Adding clang-format to pre-commit YAML config. #182

Conversation

balancap
Copy link
Contributor

@balancap balancap commented Sep 4, 2024

Using the same clang-format version v17.0.6 as XLA project:
https://github.com/openxla/xla/blob/main/.github/workflows/clang_format.yml

@jakevdp I believe the project C++ codebase is small enough to directly add clang-format to pre-commit hooks. Beyond making sure every PR is well formatted, it also has the benefit of making sure everyone is using the same version (aligned on XLA codebase).

@jakevdp
Copy link
Collaborator

jakevdp commented Sep 4, 2024

Hey, thanks for this!

I'm not sure we'll be able to merge this, because this repo is mirrored to Google's internal repo, which uses formatters that may differ in the details. When two formatters disagree on a change, it causes problems because it's often difficult if not impossible to write code in a way that satisfies both.

I'll pull in these changes to see if it causes any issues – if not, then we can probably try this out (though if it causes problems in the future I'll probably revert this change).

What do you think?

@jakevdp jakevdp self-assigned this Sep 4, 2024
@copybara-service copybara-service bot merged commit 17a83f1 into jax-ml:main Sep 4, 2024
12 checks passed
@balancap
Copy link
Contributor Author

balancap commented Sep 4, 2024

Thanks for the quick review, no worries if you need to revert it in the future (hopefully if we keep the same clang-format version as XLA it should be fine?)

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

Successfully merging this pull request may close these issues.

2 participants