Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix pre-commit hook; add CMake formatting pre-commit #759

Merged
merged 9 commits into from
Dec 10, 2024

Conversation

jwallwork23
Copy link
Contributor

@jwallwork23 jwallwork23 commented Dec 10, 2024

Fix pre-commit hook; add CMake formatting pre-commit

There was a minor issue in the pre-commit hook because the final lines checked for differences in nextsim_precommit_before.patch and nextsim_precommit_after.patch but these were replaced with calls to mktemp.

While fixing this, I realised we aren't currently formatting CMakeLists.txt files but tools for this exist. Let me know if this is something we'd like to include.

I also added some docs on how to get the pre-commit hook set up.

(Almost all of the changes in this PR are auto-formatting.)

@jwallwork23 jwallwork23 added bug Something isn't working enhancement New feature or request labels Dec 10, 2024
@jwallwork23 jwallwork23 self-assigned this Dec 10, 2024
@timspainNERSC
Copy link
Collaborator

I like the idea of autoformatting the CMakeLists.txt. But I don't like the formatting changes included in this PR. I like the lists of sources (&c.) with one item per line. I think it's easier to read and also easier to add or remove items, as this corresponds to adding or removing lines, rather than editing lines.

@jwallwork23
Copy link
Contributor Author

I like the idea of autoformatting the CMakeLists.txt. But I don't like the formatting changes included in this PR. I like the lists of sources (&c.) with one item per line. I think it's easier to read and also easier to add or remove items, as this corresponds to adding or removing lines, rather than editing lines.

Let me see if I can set configure options for that.

Comment on lines +33 to +36
# NOTE: We ignore C0103, which would enforce that variables are all-caps
# NOTE: We ignore C0113, which would enforce that comments are used
# NOTE: We set the tab size to 4
# NOTE: We set the maximum line length to 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I figured out how to set a config file with these four edits. With these edits, the lint check passes without modification of the code!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was generated with

cmake-lint --disabled {C0103,C0113} --tab-size 4 --line-width 100 --dump-config python > .cmake-lint-config.py

so it consists of the defaults plus the four edits. I figure it'd be useful to use the current defaults, given our experience with the default settings of clang-format changing.

Copy link
Collaborator

@timspainNERSC timspainNERSC left a comment

Choose a reason for hiding this comment

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

👍

@jwallwork23 jwallwork23 merged commit 35bebf9 into develop Dec 10, 2024
5 checks passed
@jwallwork23 jwallwork23 deleted the fix-precommit branch December 10, 2024 10:55
@jwallwork23 jwallwork23 mentioned this pull request Dec 10, 2024
jwallwork23 added a commit that referenced this pull request Dec 13, 2024
# Workflow handling

In the interests of green computing, development efficiency and avoiding
unnecessary CI runs, this PR sets up paths for workflow handling, so
that the linters are only run if the source code or build code change
and the test suite is only run if the test suite, build system, or
config files change.

The PR also fixes a copy-paste error in the linting workflow (introduced
in #759).
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

2 participants