Skip to content

Add .clang-format config, provide to super-linter, and apply formatting to codebase #439

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

Merged
merged 3 commits into from
May 16, 2025

Conversation

jackbondpreston-arm
Copy link
Contributor

@jackbondpreston-arm jackbondpreston-arm commented May 13, 2025

55b6bf7 applied clang-format to the codebase, but did not add the configuration file used. This change adds the configuration file, .clang-format, and applies clang-format to the codebase. The configuration file is imported from PyTorch, with attribution in comments at the beginning of the file.

The config file used seems to line up well with the one used for 55b6bf7, as the diff generated is relatively small. Having the config available will enable contributions going forward to use consistent formatting and avoid having to apply it across the whole codebase again.

To further this aim, I have linked the .clang-format into .github/config/lint, to enable the lint job in CI to utilise it for formatting checking. Additionally, I have updated the version of super-linter used in this job to a newer one, as this provides a much newer version of clang-format.

I have verified the super-linter job locally, using act, and have confirmed it seems to be working fine. For this reason, I suggest that we can re-enable the linting job in CI, to ensure contributions now follow the .clang-format.

The benefit to this project is that we can have a consistent and clear formatting style, saving time for developers and reviewers by avoiding having to think/argue about formatting - it can simply be applied with a standard tool.

.clang-format configuration file added to allow standardisation of formatting
across the repository.
The configuration was obtained from PyTorch. The PyTorch license file is
included as a comment in the file, to ensure copyright holders are given
attribution.
Updating super-linter gives a significantly newer version of clang-format
(17.0.6 -> 19.1.4).
Linking .clang-format to .github/config/lint/.clang-format allows super-linter
to use the configured rules for linting.
This commit extends the work done in 55b6bf7.
clang-format (version 19.1.7) is applied to the whole Gloo codebase. The config
used is the .clang-format file provided in the previous commit, so that a
consistent format can be used for all contributors going forward.
Copy link
Member

@d4l3k d4l3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Would be nice to setup a lintrunner CI job as well but this is a good start

@jackbondpreston-arm
Copy link
Contributor Author

Would be nice to setup a lintrunner CI job as well but this is a good start

What does this mean, compared to the existing super-linter job?

@d4l3k d4l3k merged commit 4854c8d into pytorch:main May 16, 2025
7 checks passed
@d4l3k
Copy link
Member

d4l3k commented May 16, 2025

@jackbondpreston-arm Doesn't seem like the super-linter CI job is running? Need to follow up on that

@jackbondpreston-arm
Copy link
Contributor Author

@jackbondpreston-arm Doesn't seem like the super-linter CI job is running? Need to follow up on that

Yes, I imagine it was disabled due to it previously not having any .clang-format configuration for clang-format linting. This would have caused tonnes of failures, as Gloo's formatting doesn't line up with clang-format's default config - which is likely why it got disabled. I addressed this issue in this patch, and confirmed that (at least locally, using act) the super-linter job is working as expected again.

So, I think we should be good to re-enable the super-linter job.

@jackbondpreston-arm jackbondpreston-arm deleted the clang-format branch May 16, 2025 18:13
@d4l3k
Copy link
Member

d4l3k commented May 16, 2025

Looks like it's working in #445

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.

3 participants