docs: Update submitting guidelines

[nilason]

Updates submitting guidelines, following recent additions of clang-format and pre-commit.

[nilason]

Also added note on how to disable (uninstall) pre-commit.

[nilason]

I believe this is ready for merge.

[wenzeslaus]

Looks good in general and very good changes overall. Thanks!

[nilason]

I added a part to submitting_c.md on using grass_clang_format.sh, please check.

[wenzeslaus]

Great. Thanks!

[wenzeslaus]

Partially, but only partially, off topic: The pre-commit is not enforced, but it would be a shame to have unsynced rules in pre-commit and in CI. Did you/can you look into pre-commit run --all-files for CI (in addition to what we are running now)?

[nilason]

Partially, but only partially, off topic: The pre-commit is not enforced, but it would be a shame to have unsynced rules in pre-commit and in CI. Did you/can you look into pre-commit run --all-files for CI (in addition to what we are running now)?

This is the current status:

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
markdownlint.............................................................Passed
black....................................................................Passed
flake8...................................................................Failed
clang-format.............................................................Passed
yamllint.................................................................Passed

Flake8 fails with --all-files because in that mode it doesn't respect .flake8 (we could add all exceptions to .pre-commit-config too, then we have to keep the two in sync), works however with ordinary commit, but otherwise present state is good. This after the clang-format PRs, #2785, #2762 and #2790.

I've been a bit concerned about how to enforce the "trim trailing whitespace", "fix end of files", "markdownlint" and "yamllint", the super-lint settings/abilities isn't/can't be in absolute sync with pre-commit. For example, pre-commit's markdownlint errors with too long lines, whereas super-lint only issues warnings, see eg. the RFC PRs (the present state of them wouldn't pass pre-commit).
I've been looking briefly on pre-commit-ci, which could as part of an action even "add" a commit to PRs applying the "fixing-hooks" like "trim trailing whitespace" or "black", but I'm not quite sure how it works in practice.

[wenzeslaus]

I think it's okay when let's say Super-Linter is less strict if pre-commit runs in CI too with a reasonable error message.

The pre-commit doc says:

pre-commit can also be used as a tool for continuous integration. For instance, adding pre-commit run --all-files as a CI step will ensure everything stays in tip-top shape. To check only files which have changed, which may be faster, use something like pre-commit run --from-ref origin/HEAD --to-ref HEAD

However, ignoring .flake8 is not good at this point given the amount of exceptions/ignores we need to have now.

[wenzeslaus]

...I mean good in the sense that you can't simply run all the pre-commit checks on all files in the CI, but otherwise it works just fine I think.

[nilason]

Usually there is no need to run on all files, why would there be? You only check the modified files for commit (staged), no need to run pre-commit explicitly.

[echoix]

I'm on the side that if these style checks must be enforced in order to integrate changes to the code, it should be a job for continuous integration. And rather than ask everyone to run a specific script to have a certain output, why don't we simply make it on the CI that applies the changes directly to the PR (fixes).

It was from a discussion in one of the grass repos last year that I learned about Megalinter and Super-Linter, and these "fixes" I know that MegaLinter makes it possible to commit them in the PR, if a linter provides fixes automatically. I have been contributing to MegaLinter since that time I learned about it (this and grass are my two actively followed projects). I also know that at least for the MegaLinter side, it makes it possible to only run on the new/modified files, and Super-Linter might do this too, since they were the same project a couple years ago.

I see precommit as a more personal tool, but haven't really used that framework yet to have a strong opinion.

[nilason]

Every push to GitHub here on grass is very “costly” in CI time and the slots easily filled. Therefore I’d say it is should be recommended to pre-empt unnecessary pushes using pre-commit. All the same, “fixable” hooks would be good to make CI take care of, just in case.

[echoix]

Every push to GitHub here on grass is very “costly” in CI time and the slots easily filled.

I agree that the CI is quite long, there is a lot of duplicated building, and it's not "fast" either. There can be some work on there to be done if it's really a priority for the project 😃 Correct me if I'm wrong, but the concurrent CI instances are shared between OSGeo repos, right? I seem to understand a lot of time is spend on waiting to run the checks.

Therefore I’d say it is should be recommended to pre-empt unnecessary pushes using pre-commit. All the same, “fixable” hooks would be good to make CI take care of, just in case.

Completely agree with both of these points

[wenzeslaus]

Just to be clear, this can be merged, but this discussion is good to have!

...the concurrent CI instances are shared between OSGeo repos...

Yes, I think it is per organization although I don't see that explicitly stated in the docs.

...a lot of time is spend on waiting to run the checks.

That's most of the time on a "bad" day (bad meaning very active, so good :-)). The runs are all under 2 hours, most under 20 minutes. That's not to say that faster or less jobs would not be better. Also, @echoix if you want to add MegaLinter or replace Super-Linter, go for it. If I recall correctly, the only reason we have Super-Linter and not MegaLinter is that MegaLinter did not exist yet.

Therefore I’d say it is should be recommended to pre-empt unnecessary pushes using pre-commit.

...or editor settings. For example, some QtCreator plugin can apply clang-format on file save. However, pre-commit is much easier to set up than many different checks.

All the same, “fixable” hooks would be good to make CI take care of, just in case.

I'm little hesitant about that because it makes it harder for people not familiar with Git - something else is changing their branch and they need to update. I'm using it in https://github.com/ncsu-geoforall-lab/grass-gis-on-hpc-henry2/ and I have mixed feelings about it mostly because I was not able to configure my local tools same as MegaLinter.

BTW, this is on the first Google page of "pre-commit enforced in ci":

Pre-commit hooks are a waste of time.

Why? Because they enforce standards too early. Pre-commit hooks make us wait for a quality check on every commit.

I don't necessarily agree (will see in some time), just something to be aware of.

[nilason]

Just to be clear, this can be merged, but this discussion is good to have!

Figured that much :-) , done!

I'm little hesitant about that because it makes it harder for people not familiar with Git - something else is changing their branch and they need to update.

I agree and that is also something I have been considering on the con side of CI commit hooks.

BTW, this is on the first Google page of "pre-commit enforced in ci":

Pre-commit hooks are a waste of time.

Why? Because they enforce standards too early. Pre-commit hooks make us wait for a quality check on every commit.

I think this more reflects the use of pre-commit locally, not on CIs. Ideally a "pre-push" hook, triggered on git push would perhaps be less disturbing than on git commit, but I'm not aware of such hook.

[wenzeslaus]

I think this more reflects the use of pre-commit locally, not on CIs.

Yes, I think the point there was CI time is still cheaper than developer time. (Plus you can't use pre-commit for enforcement anyway.) Our pre-commit is fast right now, so no trouble there.

Ideally a "pre-push" hook, triggered on git push would perhaps be less disturbing than on git commit, but I'm not aware of such hook.

I did not investigate, but pre-commit doc mentions push and this SO refers to the Git feature.