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

Code formatting #222

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Jul 9, 2024

Overview

As a followup to #218, This introduced a basic configuration file for the pre-commit software that calls clang-format and ruff run code-formatting on all new c/c++/python files added to grackle. All existing files will not be touched. (with that said, I tested out the new c/c++ linting on src/clib/utils.c and src/clib/utils.h since they are unlikely to have merge conflicts).

I also added documentation explaining how to locally run this machinery.

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 ruff and 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 we need to do after merging

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

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Aug 8, 2024

I made a few tweaks based on the work I had done while creating similar a similar PR for Enzo-E (in enzo-project/enzo-e#420)

These changes include:

  • modifying .clang-format formatting rules to prevent reordering of include-directives (I think it might be nice to re-enable this, but that's a topic for a different day).
  • I moved most excluded files from the .pre-commit-config.yaml file to tool-specific files (.ruff.toml1 and .clang-format-ignore2). This ensures that the linting tools are aware of the exclusion list, even if you manually invoke the linting tools (outside of pre-commit)
  • I improved the documentation.

Footnotes

  1. Once we merge Transition Pygrackle from setuptools to scikit-build-core #208, we should move the contents of .ruff.toml into the [tool.ruff] section of the pyproject.toml file that will be introduced.

  2. I also needed to bump the clang-format version used during linting (the previously pinned version didn't know about the .clang-format-ignore file)

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Sep 9, 2024

I added a 3.4 milestone to this (but we can change it). My primary motivation for doing this was Britton's idea that we could start more broadly applying formatting after the next release.

@mabruzzo mabruzzo added this to the 3.4 milestone Sep 9, 2024
@mabruzzo mabruzzo added code style Related to linting tools ci Related to Continuous Integration labels Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to Continuous Integration code style Related to linting tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant