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

build: introduce pre-commit framework #850

Open
wants to merge 1 commit 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
@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 ahans force-pushed the feature/ahans/pre-commit branch from 3800ebb to 411c53c Compare April 3, 2024 19:39
@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.

@ahans ahans requested review from ErikSchierboom and vaeng April 3, 2024 19:50
@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.

@ahans ahans force-pushed the feature/ahans/pre-commit branch from 411c53c to a75ab3d Compare April 9, 2024 09:23
@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 ?

@vaeng
Copy link
Contributor

vaeng commented Jan 28, 2025

Are you back with some more time to discuss this @ahans @siebenschlaefer?

@ahans
Copy link
Contributor Author

ahans commented Jan 29, 2025

Are you back with some more time to discuss this @ahans @siebenschlaefer?

Except for the formatting issue mentioned above I think this is done. Let me rebase and implement my proposal that includes the TODO comment and then we can merge. I will try to find some time in the next couple of days.

@vaeng
Copy link
Contributor

vaeng commented Feb 3, 2025

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.

This is a good addition for the students. I would rather have the comment in the block and have the other places neat and tidy.

@ahans ahans force-pushed the feature/ahans/pre-commit branch from a75ab3d to e7741a7 Compare February 23, 2025 22:02
@ahans ahans force-pushed the feature/ahans/pre-commit branch from e7741a7 to b132101 Compare February 23, 2025 22:41
@vaeng
Copy link
Contributor

vaeng commented Feb 24, 2025

This looks really clean now. Although I have to admit that I did not look through all the changes.

@ahans
Copy link
Contributor Author

ahans commented Feb 24, 2025

This looks really clean now. Although I have to admit that I did not look through all the changes.

I still need to make sure that the test runner is able to parse all test case names. Once that is out of the way, I think we can merge this.

I noticed there are quite a few files that are missing EOL. GH flags this with ⛔ and there's another pre-commit that takes care of this. But as I said earlier, we can easily do that via a follow-up PR to keep individual PRs as small as possible.

@ahans
Copy link
Contributor Author

ahans commented Feb 25, 2025

I ran the test runner locally on the exercises, they all still pass, so looks like no formatting issues with long lines there. It is good to merge from my point of view. However, I was wondering if I should include the [no important files changed] in the commit message? After all, it's only formatting changes, so validity of students' solutions should not be affected? @vaeng wdyt?

@vaeng
Copy link
Contributor

vaeng commented Feb 25, 2025

Yes, I am with you. We should merge with [no important files changed].

@ahans ahans force-pushed the feature/ahans/pre-commit branch from b132101 to c37b13b Compare February 25, 2025 14:36
@ahans
Copy link
Contributor Author

ahans commented Feb 25, 2025

I find it unfortunate that I have to squash this to a single commit. Having it in two separate commits I find much cleaner. What's up with this rule?

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

This commit also applies the fixes from `pre-commit run --all`, but
modifies the changes to solution templates to contain a comment, so that
we don't end up with `namespace foo {}` lines.

[no important files changed]
@ahans ahans force-pushed the feature/ahans/pre-commit branch from c37b13b to abb62e4 Compare February 25, 2025 15:55
@ahans ahans changed the title feat: introduce pre-commit framework build: introduce pre-commit framework Feb 25, 2025
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