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

feat: introduce pre-commit framework #850

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahans
Copy link
Contributor

@ahans ahans commented Mar 24, 2024

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

@ahans ahans requested a review from a team as a code owner March 24, 2024 16:15
Copy link
Contributor

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 github-actions bot closed this Mar 24, 2024
Copy link
Contributor

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!

Copy link
Contributor

@vaeng vaeng left a comment

Choose a reason for hiding this comment

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

I did not check every single file, but it seems to work on the ones I tried.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed for the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. I opened this accidentally against this repo instead of my fork. It's not cleaned up in any way yet. Just wanted to showcase how it could look like. I can clean up this PR and let you know when I'm done. Then you can do a proper review.

Copy link
Contributor Author

@ahans ahans Apr 2, 2024

Choose a reason for hiding this comment

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

That file had some whitespace violation. I removed those checks now from this PR. This now is strictly only clang-format.

@vaeng vaeng reopened this Mar 26, 2024
.github/CODEOWNERS Outdated Show resolved Hide resolved
@ahans ahans force-pushed the feature/ahans/pre-commit branch 2 times, most recently from ea7e32d to 3800ebb Compare April 2, 2024 20:35
@ahans ahans changed the title Feature/ahans/pre commit feat: introduce pre-commit framework Apr 2, 2024
@ahans
Copy link
Contributor Author

ahans commented Apr 3, 2024

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 vaeng added x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) x:rep/large Large amount of reputation labels Apr 9, 2024
@vaeng
Copy link
Contributor

vaeng commented Apr 9, 2024

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
Copy link
Contributor

vaeng commented Apr 9, 2024

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

@ahans
Copy link
Contributor Author

ahans commented Apr 9, 2024

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.

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
Some tests get manual formatting and local `clang-format off` to work around
exercism/cpp-test-runner#73
@ahans
Copy link
Contributor Author

ahans commented Apr 9, 2024

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
Copy link
Contributor

vaeng commented Apr 9, 2024

We could fix this with a comment:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:rep/large Large amount of reputation x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants