Adds linting via pre-commit

[wusatosi]

Propagates linting infrastructure.

Updated it to better fit beman:

1. Adds codespell (auto fix not enabled)
2. Update Markdown column limit to default (80) as this is more of a documentation library. Keeping Code block column size 120 to conform with norms in across projects so we can still copy-paste code over.

Updated all files so it passes linting.

[steve-downey]

👍

[steve-downey]

We might later want to have a conversation about sentence or paragraphs on single line, so as to reduce diff noise, but it does require friendly editor configuration.

[wusatosi]

We might later want to have a conversation about sentence or paragraphs on single line, so as to reduce diff noise, but it does require friendly editor configuration.

Yeah let's have this setup before it's too late...

Actually, @steve-downey do you want me to create a .git-blame-ignore-revs for this?

[steve-downey]

Yes, as long as we're merging and not squashing, otherwise we just have to do it again.

[wusatosi]

Yes, as long as we're merging and not squashing, otherwise we just have to do it again.

Can you look over the formatted file? I think I will squash all the formatting commits and write that single commit hash into .git-blame-ignore-revs.

Just glance through this and see if I have made any dumb mistakes.

[JeffGarland]

I'm wonder if this is really what we want? Aside from the obvious inefficiencies involved in checking all files all the time this will make it potentially difficult to override a lint/format setting. Let me give you a concrete case of this. In my day-job-project we using clang-tidy on code, but it struggles in any setup we've used to nicely format monadic function chains. So if we had are setup run all files every checkin it would be annoying (note there's likely ways to turn off the changes we don't know about). Looking for other opinions on this point.

[wusatosi]

Upstream for this CI config is in exemplar, so the context is this is meant to be run along side some huge unit test complex.

The main idea is if we have a linting rule (or any enforceable rule) then if some part of the system is broken, the whole system is broken. Linting potentially causes sweeping changes due to reformatting, so its best the reformat is as close to lint error as possible (best in the same commit).

Explicitly, we are trying to avoid this:

• commit 0 feature [x diff breaks lint a.cpp]
• commit 1 feature [✓ diff pass lint b.cpp]
• commit 2 ready to ship [✓ diff pass lint b.cpp]

Here the lint failure in 0 may still not be fixed because no diff later touches a.cpp .

• This is inefficient: Yes this is inefficient, but all the linters are light weight and often doesn't even involve the need to construct an AST. In comparison to pulling the packages to prepare linting, the unit test of cross platform + cross compiler matrix/ later the long clang-tidy job (in exemplar/ C++ project context), the actual lint cost here is minimal. Arguing lint cost here is in efficient.
• Override linting: This is not the concern of CI, overriding linting should be the concern of configuring the linter. e.g. codespell:ignore, markdownlint-disable-next-line, gersemi: ignore. Linting error should be explicitly suppressed (best in-line), but not to be ignored. Running lint check only on diff doesn't guarantee error propagation unless we do complicated trickery. Think this as unit test, you would ideally run the entire unit-test set, and the solution to flaky unit tests are, ideally to fix them first, if it doesn't work then skip, but not to ignore them from CI perspective out-right.

[JeffGarland]

Thanks for the long explanation. And while I agree there are overrides possible many of these involve actual source modifications which are sometimes unpleasant for authors, etc. And, if I'm not mistaken the standard we're setting here will require the revisiting of those standards on each commit for unchanged source.

Upstream for this CI config is in exemplar, so the context is this is meant to be run along side some huge unit test complex.

Sure. I'll note however, that none of this is explained in exemplar at the moment.

The main idea is if we have a linting rule (or any enforceable rule) then if some part of the system is broken, the whole system is broken.

To me there's a large difference between a broken test and some code that doesn't pass formatting checks. So maybe that makes these 'less enforceable rules'.

I guess at a minimum I'd like some others to chime in on this.

@steve-downey @neatudarius @camio - thoughts?

[camio]

I agree with @wusatosi that enforcement of linter checks at CI time is the most desirable option and this is what we're doing with every other repository in beman so far. We "push left" by allowing developers to get this feedback with the pre-commit tool, but if they don't use this, there's review dog to suggest the changes needed at PR time. I've worked in several repositories that enforce linters and it's been a positive experience.

[JeffGarland]

interesting -- I assume the linter is complaining about the dashes here?

[wusatosi]

Yeah it's designed to enforce a consistent listing symbol, and the default is *, I think I could switch it to -

[JeffGarland]

Suggested change

	The Beman project is named in memory of Beman Dawes - co-founder of
	The Beman project is named [in memory of Beman Dawes](https://www.bemanproject.org/2024/10/30/about-beman/) - co-founder of

[wusatosi]

We should delegate this to another PR, let's keep this PR strictly formatting...

[wusatosi]

@camio I squashed all formatting commits into a single commit and added it into the .git-blame-ignore-revs (git blame is working on GitHub and on my local git instance) per previous conversation with @steve-downey .
I also bumped dependencies for the pre-commit hooks.

You can merge this PR if you accepts the new changes.

[camio]

@wusatosi, I can't "rebase and merge" this because there are conflicts. Can you please fix the conflicts, rebase, and let me know when that's done so I can go ahead and merge this?

[wusatosi]

@wusatosi, I can't "rebase and merge" this because there are conflicts. Can you please fix the conflicts, rebase, and let me know when that's done so I can go ahead and merge this?

I can merge this, I assume the git blame ignore is desired?

Do I have to do rebase merge? Won't it change the commit hash so that git blame ignore won't work?

[steve-downey]

If the merge is otherwise clean I don't see a reason to rebase it, and as you said, it would invalidate the blame ignore.

At work, earlier last week, I got asked about some tool because I was the only one in the commit history, however the message on the commit was "restore accidental deletion of trunk", and the earlier commits in CVS didn't survive the migration. Dropping tool commits from blame is a good idea.

[wusatosi]

Hey @camio this PR is getting increasingly outdated from the main branch due to the long delay in review.

Given we would ideally like to keep the git blame ignore file small, I will need to rebase and squash so that main formatting commit cover updated changes.

If squashing is not needed, rebase main still causes the need to double check changes made during initial reformat against subsequent changes happened during the review process, to ideally check in their formatted counterpart. This essentially means almost every single commit happened throughout this review process is a conflict and making resolving them very error prone.

We don't have an inplace auto-formatter for markdown, formatting has to be done manually by hand. I would very much prefer if we can minimize duplicated work.

Rebase doesn't make sense here, is error prone, and is essentially redoing this PR again.

Given there's numerous pending PRs in the review queue, it doesn't make sense for me to merge main on every change if this PR doesn't feedback immediately. I will stop updating PR to keep up with main for now until I get feedback on the following questions.

Can you simply provide input on:

1. If I can simply merge instead of rebase
2. Confirmation the git blame file is desired and well formed.

If this is desired, I will merge main, resolve the conflict with the formatted counterpart, update git blame file, and merge.