search

exercism / cpp

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

Formatting check in CI is broken #834

Open ahans opened 4 months ago

ahans commented 4 months ago

The bin/check-formatting.sh currently expects a path relative to bin (or absolute), but it's called in CI with paths relative to the repo root. So no checks are run. Since currently not finding a file doesn't cause a non-zero exit code, the checks still pass.

ahans commented 4 months ago

A fix for the issue is here: https://github.com/exercism/cpp/pull/835

ahans commented 4 months ago

However, there's another issue in the check of all files never happens. It is supposed to be run for each commit on main. However, since the if in the workflow checks for a branch named master, this never becomes true (it's called main). Fixing that is trivial of course, but we should probably also re-format all files in the same PR.

I can certainly do that. Just let me know. There will be a few hundred files that need reformatting.

vaeng commented 3 months ago

I can certainly do that. Just let me know. There will be a few hundred files that need reformatting.

Please go forward, this is very good for the code base. Do you know if we could easily check the correct formatting for the code blocks in the .md files?

ahans commented 3 months ago

I can certainly do that. Just let me know. There will be a few hundred files that need reformatting.

Please go forward, this is very good for the code base.

Alright, I can do that. However, that should come after the check is in place. So could you have a look at #835 please? That fixes the script and will make sure that future PRs are properly formatted.

Fixing the check that runs for each commit on main and reformatting existing files I would do in a follow-up PR.

Do you know if we could easily check the correct formatting for the code blocks in the .md files?

I haven't done this yet, but clang-format-docs looks promising. I have just run it on a random concepts/headers/introduction.md and it looks like it's working properly. Running the formatting on .md files and putting a CI check in place I would also do in a follow-up PR then.

Instead of extending the existing script or adding another custom script to do this, would you be open to use pre-commit for this? I have had quite positive experiences with this in other projects. Although it was meant to be run as a Git hook originally, you don't have to use it like that. The current workflow can stay in place, but instead of a custom script, you would run pre-commit run locally to run the reformatting tool(s).

vaeng commented 3 months ago

Pre commits only work, if all people who make a PR actually use them, right? I am not against this, but I would argue that we have to have correct files even if the PR is from someone who does not check with a pre-commit hook before they send it in.

ahans commented 3 months ago

Pre commits only work, if all people who make a PR actually use them, right? I am not against this, but I would argue that we have to have correct files even if the PR is from someone who does not check with a pre-commit hook before they send it in.

No, you don't have to use it as a Git hook. That's how it started out and hence the name. But I personally don't use it like that in any project. Instead, I trigger it manually by running pre-commit run. That then runs whatever is configured. To replicate the current configuration of this project, we would only have clang-format. But clang-format-docs could be easily added and would then run as well. pre-commit run returns a non-zero exit code in case it made any changes. That is what would be used in CI, so that all configured checks are also part of a CI run. So no matter what people use locally to create PRs, we would have all checks in place. The big upside is that pre-commit is a widely used project, so the probability of things not really working (as has happened now with the custom script) is really low.

You know what, let me put together a PR today or tomorrow to show you how it would look like.

ahans commented 3 months ago

Alright, so I put it up for my fork and it seems to be working fine. The commit messages would need some cleanup, but everything is there to get the idea.

The PR is here. pre-commit is configured via .pre-commit-config.yaml. In addition to the clang-format check, there are a few standard checks. Those are not mandatory, but don't hurt either, so I kept them in. So this config plus the pre-commit command is all you need. Install pre-commit locally and you're good to go. My typical workflow is running pre-commit run --all locally. Without the --all it only looks at the current commit, so that's what would be used in a Git hook (via pre-commit install you can locally create the hook, but that's optional).

For the CI setup I created an extra workflow. That just uses the pre-commit action. I noticed in the meantime that it always runs with --all, so that special handling of PR vs main is not necessary and should be removed. If you want to see a CI run where the check fails, here is an example.