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

Introducing pre-commit configuration to call clang-format #404

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Jul 3, 2024

Overview

This introduced a basic configuration file for the pre-commit software that calls clang-format.

In general, pre-commit is a tool written in python that was originally designed to be use locally (any user of cholla is free to do that). In the original design, it was intended to be used to easily attach varying "lints" to git's "commit-hooks". More recently, pre-commit has become popular for continuous integration. In principle, pre-commit can be used with any arbitrary continuous integration provider (whether it's Jenkins, GitHub Actions, CircleCI, etc.).

We are interested in using pre-commit.ci, which is a continuous integration system designed around pre-commit.

  • After we set up pre-commit.ci, a check will appear at the bottom of every PR that specifies whether the configured lints (namely just clang-format) passed or failed.
  • pre-commit.ci is free for all open-source software. It also has the nifty feature, where adding a comment that says "pre-commit.ci autofix" to a PR (where the pre-commit.ci check has failed), will trigger pre-commit.ci to add a commit to that PR that fixes the formatting.

yt actually makes use of pre-commit.ci

What does this PR do?

Specifically, this PR adds the configuration file for pre-commit. I have configured that pre-commit file to use clang-format (with the precise version that matches the wiki -- 17.0.1).

I also made a tweak to a file flagged by a run of pre-commit. I'm a little worried about that... But, if the existing CI test suite succeeds, then I think we are probably fine. (Clang-format will continue to run as part of Jenkins, so will know if any disagreements arise -- there really isn't any risk for Cholla). EDIT: I'm no longer worried -- the GitHub action seems to be totally fine with this change in file formatting. Plus, it looks like Jenkins was successful at applying clang-format

What we need to do after this PR is merged

We will need to go to the pre-commit.ci and enable it for the Cholla repository. (I'm happy to try to do that, but I may not have appropriate permissions -- so Evan may need to do that)

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Jul 3, 2024

Here's an example of a comment in a pull request that I made to yt a while back, where one of the maintainers used pre-commit.ci to reformat my python-code

@evaneschneider
Copy link
Collaborator

Looks like everything passed this times, so I'll go ahead and merge this and hopefully we can start using it right away!

@evaneschneider evaneschneider merged commit cb3d2d5 into cholla-hydro:dev Jul 3, 2024
10 checks passed
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.

2 participants