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

Automatic code formatting in PRs #456

Closed
2 tasks done
Tracked by #455
jrhemstad opened this issue Sep 18, 2023 · 5 comments
Closed
2 tasks done
Tracked by #455

Automatic code formatting in PRs #456

jrhemstad opened this issue Sep 18, 2023 · 5 comments
Assignees

Comments

@jrhemstad
Copy link
Collaborator

jrhemstad commented Sep 18, 2023

We want an easy, automatic system for checking code in a PR is consistent with our .clang-format as well as applying the appropriate formatting.

An important part of maintaining a consistent formatting is using the identical version of the clang-format binary everywhere. We can address this by including a consistent version in all of our devcontainers.

However, for the convenience of external contributors, we should also have a way to automatically format the code without requiring them to format it locally. One option would be to have an action that automatically checks if the code is formatted correctly and immediately commits to the PR with the correctly formatted code.

However, this may be surprising to people to have commits automatically added to their PR without manual intervention. As a compromise, we should still add a Action to verify the code is formatted, but instead of automatically applying the formatting, we add a /format slash command that will format the code and apply a commit to the PR.

Tasks

  1. feature request
    bdice
@miscco
Copy link
Collaborator

miscco commented Sep 20, 2023

I believe this is essentially blocked on us providing a explicit formatter in the devcontainer and configure it such that formatting is enabled in the repo.

Looking at the llvm feature we are currently installing all packages. @trxcllnt would it be feasible to create a "clang-format" feature?

What I really want to avoid is that the llvm-16 devcontainer requires different formatting than the llvm-7 devcontainer because it uses different clang-format binaries.

Furthermore, if we enable code checking as part of CI we need to also provide a way for users to automagically do this. I do not care the slightest whether we use cmake-format or a pre-commit hook or whatever, but it must be frictionless to use within all our devcontainers

@jrhemstad
Copy link
Collaborator Author

I believe this is essentially blocked on us providing a explicit formatter in the devcontainer

Yep, that's the first item in #455

What I really want to avoid is that the llvm-16 devcontainer requires different formatting than the llvm-7 devcontainer because it uses different clang-format binaries.

Indeed. We'll install the latest version of clang-format in all the images.

provide a way for users to automagically do this

That's what the slash command is for 🙂.

@miscco
Copy link
Collaborator

miscco commented Sep 20, 2023

I meant to do it locally and not in CI later

@jrhemstad
Copy link
Collaborator Author

Local formatting will be done with devcontainers. If people don't want to use devcontainers, then we'll have the slash command.

@trxcllnt
Copy link
Member

Yeah, I can make the LLVM feature only install a subset of the packages. Then we can define it multiple times with different versions and package subsets. I just did this with the CUDA feature so we could layer in cuTensor, cuDNN, etc. So this shouldn't be too difficult.

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

No branches or pull requests

4 participants