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 Additional Formatting Continuous Integration #218

Open
mabruzzo opened this issue Jul 8, 2024 · 3 comments
Open

Introducing Additional Formatting Continuous Integration #218

mabruzzo opened this issue Jul 8, 2024 · 3 comments
Labels
ci Related to Continuous Integration code style Related to linting tools proposal

Comments

@mabruzzo
Copy link
Collaborator

mabruzzo commented Jul 8, 2024

With the recent flurry of development, I think it could be useful to introduce some additional Continuous Integration to further improve code uniformity. Inspired by the usage of pre-commit.ci in yt, I recently introduced pre-commit.ci to the Cholla code-base with great success. The capability to explicitly trigger pre-commit to automatically fix

I think that would be useful to introduce here (unless people strongly object).

In particular there are 2 things to consider:

  • C/C++ formatting (I think this is most important important given the large additions to the C code):
    • the standard tool is clang-format. It acts a lot like black, but for C/C++. The main difference is that clang-format is extremely flexible and configurable. Basically, the standard thing to do is just pick a standard style that we like and adopt it.
    • I personally like the Google style or Mozilla style. Here is an online tool that illustrates different code-formatting styles. Do people have opinions about this?
    • Note: the versioning of clang-format is tied to the clang compiler (and the rest of the llvm project) and unfortunately, things are not perfectly backwards/forwards compatible. We would pick a version of clang-format and stick with it for a little while (This is arguably a reason to pick standardized style). Part of the beauty of pre-commit is that we can easily ensure everyone uses a consistent version of the formatter.
    • Given the fragmented nature of Grackle development, I worry this might increase contribution-friction from existing branches. A nice compromise is to only apply the formatter to new files, for now (If we adopt this philosophy, I actually think now is the time to do this since there are relatively few c/c++ files in repository -- I think we are about to introduce a bunch of new ones).
  • python/cython formatting:
    • I feel a little less strongly about this (since we already have flake8), but I think this could still be useful
    • for python we could adopt black or ruff. (not sure which is better)
    • for cython formatting: I know that yt adopts these hooks
@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Jul 8, 2024

I guess the 2 major topics for discussion:

  • do people have feelings for or against? (strong objections?)
  • how about feelings about c/c++ styles? I'm really tempted to forge ahead with the Google style. It is arguably very similar to what we have and it seems to be an extremely common choice for open-source astronomy software. The beauty of adopting a formatter is that we could change the style later.

@mabruzzo mabruzzo added proposal code style Related to linting tools ci Related to Continuous Integration labels Jul 8, 2024
@brittonsmith
Copy link
Contributor

I am generally in favor of this and I definitely agree the time to do it is before there is a lot of divergent development. I am wary of introducing merger conflicts, so perhaps starting with new files and then doing everything after, say, the next release or so would be a good idea. I don't have strong opinions on which style we go for. My understanding is that yt is moving to ruff, so maybe it's worth following them.

@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Jul 9, 2024

Sounds great. I'll open a PR to do this. In that PR, I'll:

  • exclude all the old files
  • adopt ruff -- I actually only know it exists because I saw that yt was moving to it :)
  • make "a choice" about c/c++ style (probably the google style-guide since it seems fairly standard). If people have opinions about that, we can always break that part of the PR off into a separate piece

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 proposal
Projects
None yet
Development

No branches or pull requests

2 participants