exercism / cpp

Exercism exercises in C++.
https://exercism.org/tracks/cpp
MIT License
251 stars 205 forks source link

feat: introduce pre-commit framework #850

Open ahans opened 3 months ago

ahans commented 3 months ago

To avoid confusion, I updated this PR now to only use pre-commit to replace (and fix) the existing clang-format checks. Other checks can be added in follow-up PRs.

Here is the commit message from the commit that does the actual switch. The other commit reformats all the files that went through without proper formatting due to the bug in the setup:


This replaces the custom clang-format script with the pre-commit framework, making it easier to add checks in the future and simplifying GitHub integration.

The GH workflow is adapted to run checks now as well.

To use it locally, all you need to do is install pre-commit locally, typically via:

pip3 install --user pre-commit

You can then run pre-commit run --all to run it locally. Note that this does not install a Git hook. If you want that as well, run pre-commit install.

For more details on pre-commit and other installation options, see https://pre-commit.com/ and https://pre-commit.com/#install

github-actions[bot] commented 3 months ago

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

github-actions[bot] commented 3 months ago

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

ahans commented 3 months ago

I ran all practice exercises through the test runner to find the ones that needed manual formatting, because clang-format would split long test names over multiple lines. All occurrences are fixed now.

vaeng commented 2 months ago

I ran all practice exercises through the test runner to find the ones that needed manual formatting, because clang-format would split long test names over multiple lines. All occurrences are fixed now.

That would have been my final question. Thanks for all the work on this one.

vaeng commented 2 months ago

I merged your other PR. Can you please fix the conflicts, so we can merge this and have beautiful formatting?

ahans commented 2 months ago

I merged your other PR. Can you please fix the conflicts, so we can merge this and have beautiful formatting?

Ok, just saw your approval and comment here. We could have just closed the other PR without merging if we're going with this version. Using both works as well, of course.

ahans commented 2 months ago

Alright, it's rebased now. However, there was one other potential "issue" I wanted to point out. The solution templates that only have the namespace, e.g.,

namespace diamond {

}  // namespace diamond

with the current clang-format config get reformatted to look like this:

namespace diamond {}  // namespace diamond

I find this non-ideal, since a student would have to navigate inside {} and hit return there.

We could fix this with a comment:

namespace diamond {
    // TODO: implement solution
}  // namespace diamond

Then the student would still have to remove the comment, but still better than having {} IMHO.

Alternatively, we could use the clang-format option KeepEmptyLinesAtTheStartOfBlocks. I haven't tested this yet, but I think it would leave the template alone, since a namespace counts as a block in that context and the empty line would be at the beginning of the block. On the other hand, it would also leave empty lines in other places that we may not want. So I'm a bit torn on what to do.

Let me know what you think please!

vaeng commented 2 months ago

We could fix this with a comment:

I like this solution. What do you think @siebenschlaefer ?