build(lint): consolidate linters under pre-commit

[MichaelYochpaz]

This PR migrates all linting tools to pre-commit for unified local and CI execution.

This allows developers to run hatch run lint:precommit (or pre-commit run) to check for lint issues locally and auto-fix them locally, or even install git pre-commit hooks that will auto-fix them before making a commit.

Notes:

• Super-Linter which had Markdown linting (already covered by markdownlint) and editorconfig validation (now covered by editorconfig-checker) was removed.
• The .markdownlint-config.yaml config file was renamed to .markdownlint.yaml to follow the official naming convention, which is auto-used by the CLI tool used (reference). Beforehand the name was a "made up" name that was referenced by using --config .markdownlint-config.yaml.
• markdownlint required Node.js to work (we install it in the CI beforehand). The pre-commit implementation here makes so that it's required on the CI, but for local runs the markdownlint check is skipped (with a warning) if Node.js isn't installed.
• There were some unrelated files that were slightly modified. Those are files that were picked up by the pre-commit and fixed. If not updated (or more accurately, fixed to follow or linting rules), they would change whenever pre-commit is ran.

[tiran]

FWIW, I'm not a fan of pre-commit. I prefer to have linters and tests in the same automation tool, so I can run all checks in parallel from the same tool with a single command. I'm still maintaining my own tox.ini for Fromager and run ruff, linters, doc builds, and unit tests in parallel with tox p.

[MichaelYochpaz]

I prefer to have linters and tests in the same automation tool, so I can run all checks in parallel from the same tool with a single command.

@tiran This is what pre-commit does... You don't have to install the hooks, you can run pre-commit run and it will run all configured tests and linters with a single command.

[LalatenduMohanty]

Pre-commit is a significant change in developer workflow, we can skip the pre-commit failures in the local workflow incase we want to focus on the code chnages initially i.e. git commit --no-verify -m "wip: work in progress". However I am not sure if we want to adopt this workflow. I need to use this workflow and findout more.

Developer Workflow WITHOUT Pre-commit:
┌──────────┐    ┌──────────┐    ┌──────────┐    ┌──────────┐
│  Write   │ ─► │  Commit  │ ─► │   Push   │ ─► │ CI Fails │ 😞
│  Code    │    │ (no check)│   │          │    │ (lint!)  │
└──────────┘    └──────────┘    └──────────┘    └──────────┘

Developer Workflow WITH Pre-commit:
┌──────────┐    ┌──────────┐    ┌──────────┐    ┌──────────┐
│  Write   │ ─► │  Commit  │ ─► │   Push   │ ─► │ CI Pass  │ ✅
│  Code    │    │ BLOCKED! │    │ (clean)  │    │          │
└──────────┘    └──────────┘    └──────────┘    └──────────┘
                     │
                Fix & retry

[MichaelYochpaz]

Just to make sure it's clear - you are not required to install pre-commit as a git hook. It's opt-in and you have to install the hook manually locally by running pre-commit install. Only then it run automatically before every commit, autofix issues, and if there are still issues remain, block you from commiting with an error message.

Another way to use it (which is what I use, and is written in the README) is just by running it manually with pre-commit run (or with hatch run lint:precommit in our repo). Just like how you'd run pytest or black or any other linter manually with a command. It just allows to unify them all under a single command that makes it easy and automatic for developers and contributors.

[tiran]

We need a compelling reason why we should introduce yet another automation tool to run linters. pre-commit is a new tool that we have not used in any project. We would need to train all engineers on the new tool. If hatch is not sufficient for our needs, then I would rather go back to tox (or maybe look into nox).

Also I'm worried about 321 insertions(+), 116 deletions(-). That's 200 more lines to accomplish the same task.

• Fromager was using tox until @LalatenduMohanty switched us to hatch in PR Start using hatch instead of tox #627
• https://github.com/python-wheel-build/elfdeps/ from this organization uses tox
• internally we are using tox in several places like the internal wheel builder repo
• no internal or public project uses pre-commit, yet

[MichaelYochpaz]

@tiran We already have pre-commit used here on Fromager. @dhellmann added it to the repo (Link #1, Link #2).
You can see I'm just modifying the existing pre-commit config file and not creating a new one.

I'm just consolidating the linters under the same tool, and making it easier to run the linters we only have on the CI, locally.
Running pre-commit and having the official linters fix linting issues for you locally is much more convenient over pushing, waiting for the CI to run, reading the failures, and manually fixing them.

I don't think line counts should be a metric. I've used a lot of comments and updated the docs/develop.md document to be more informative. I can easily deflate that number if that was a concern for some reason.

What training would developers need? It's literally just a single command to run.
If anything, using tox would require a lot more learning and training from developers.

[dhellmann]

I like the idea of this change as a way to standardize the tool configuration. When we introduced pre-commit we only brought some of the tests in, and this makes it easier to ensure they are all run. I find that very useful when working with agents that generate code, since you can't always get them to run the linter or unit tests but you can always get them to run the pre-commit hooks when committing changes.

The markdownlint stuff is a little confusing, mostly because I don't remember how that tool is installed. Is there a way to run it via an image or to configure npm to install the right thing when it's used locally?

The only other comment is that I might have staged the changes in multiple commits to make it easier to understand what each update is doing. It's not worth splitting this up into multiple commits, now, though.

[dhellmann]

Where does this hook get the linter?

[MichaelYochpaz]

@dhellmann Yeah this allows a full sync between linters running locally and on our CI.
This is good for AI agents (they can run the full linting and unit-testing process locally with a single command and not a shopping list of different tools to run sequentially), but also humans.
I opened this after I got a long list of Markdown list errors that just referred to specific lines that I had to manually find and fix. Now we can auto-fix these locally in seconds before even pushing.

To answer your question about markdownlint: It requires Node.js to run (we install Node in our CI).
This is tricky because I want pre-commit to also work for users who don't have Node.js installed.
We could make it an optional step, but then there wouldn't be a clear message for users to know they're missing out on this linter / how to fix it (they won't know they need to install Node.js), and also, the CI (which runs the same pre-commit) would skip this lint step in case of an error because it's optional.

I thought of using containers, but than we would just require Docker / Podman instead, and it would only work with one of them (e.g., if write it as a Podman command, it won't work for Docker users). Thought it's probably too much maintenance and overhead, with many possible issues and edge cases.

The solution I came up with (which also answers your review note) is to use a script - scripts/conditional_hook.py. This script will always run on pre-commit and is required. Then inside the script, if Node.js isn't installed, there are two cases:

1. If running on the CI (detected using environment variables) - running markdownlint is required and the script will fail if Node.js is missing.
2. Otherwise (running locally) - if Node.js isn't installed, it will print a nice warning message telling users to install Node.js, and then makdownlint-cli2 using npx, then it will continue to run the other linters.

However... I just found mdformat, which is a Python-based Markdown linter with native pre-commit hooks. If we're OK with switching to another Markdown linter (which also might change some Markdown files to slightly different formatting rules this linter follows), I think this could be a much simpler and cleaner solution.

[dhellmann]

I thought of using containers, but than we would just require Docker / Podman instead, and it would only work with one of them (e.g., if write it as a Podman command, it won't work for Docker users). Thought it's probably too much maintenance and overhead, with many possible issues and edge cases.

Eh, we could just pick podman.

However... I just found mdformat, which is a Python-based Markdown linter with native pre-commit hooks. If we're OK with switching to another Markdown linter (which also might change some Markdown files to slightly different formatting rules this linter follows), I think this could be a much simpler and cleaner solution.

That's definitely worth looking at. Is the project maintained? How different is the output?

[MichaelYochpaz]

@dhellmann For public open-source projects, I prefer to make the contribution process as simple as possible, without requiring installations of tools like Podman from people who just want to contribute something quickly :)

About mkformat - looks pretty promising. It's pretty maintained (by the same guy who made the tomli package that was adopted as a standard library on Python 3.11) and has a plugin system which makes it easily extendible if we ever need to create custom Markdown linting rules or whatever.

Initially it did make some changes I'm not sure we wanted (For example: all numbered list items started with 1. instead of incremental numbers, which apparently lets Markdown parsers auto-number them), so I've updated to config and it's pretty close now.

There are still a few changes, but it's mostly converting * to - for list items, and adding a line break before lists.
You can see the changes here.

This removed a lot of overhead and complex code I previously head to deal with because of Node.js, and it's much cleaner now so I'd love to move forward with it. I've ran some tests (changing Markdown files to invalid formats) and it appears to work well.

If you'd like to test the linters, check out the branch locally, make some linting errors, and then run hatch run lint:pre-commit.

[tiran]

This block seens to fix Ruff formating and linting issues. How does pre-commit detect linting errors and fail when there is a problem?

[MichaelYochpaz]

If there are any modifications made, it returns a non-zero error code (even if all the linting issues were fixed following these changes).
And the same happens if there are issues that couldn't be auto-fixed.

[tiran]

This linter and formatter seems to replace all * bullet points with -. It's a lot of noise that interferes with git history and git annotate. The asterisk is a valid character for bullet points. I actually prefer it over dash, because it sticks out.

[MichaelYochpaz]

This linter and formatter seems to replace all * bullet points with -. It's a lot of noise that interferes with git history and git annotate

It modified only 4 doc files, and out of those only 2 doc files (docs/files.md docs/using.md) had their lists updated to use - instead of *. I wouldn't call it "a lot of noise".

I will also add that we use both * and - for different lists across our docs inconsistently, so any commit that would line these up to a specific convention would require similar changes, since we didn't enforce consistency until now and we use both symbols. See the list items here for example, that weren't modified since they already use -.

If you're concerned about git blame noise, I can move those lint changes to a separate commit (might require a separate PR since we squash merge?), and then add this commit to a .git-blame-ignore-revs and then it'll be ignored on git blame.

The asterisk is a valid character for bullet points. I actually prefer it over dash, because it sticks out.

I tried to check if there's a config option / plugin to override that, but couldn't find one.

I don't really mind either option, but if I have to choose between a Markdown linter that requires Node.js to run, and adds complexity to run locally, and a linter that just uses - for bullet points instead of * (completely valid Markdown), but runs locally and auto-fixes linting issues easily, I'll pick the other one that helps me work fast and more efficiently. I don't think this minor personal preference for docs that end up rendering the same way anyways, is worth blocking this PR.

If it's really a big concern, we could write a small plugin to override it (plugins seem pretty simple - example) but I'd rather not because we might break stuff unintentionally and it'll require maintenance if the linter updates and changes the API.

[tiran]

I wouldn't call it "a lot of noise".

I'm consider about 100 changed lines a lot of noise.

If you're concerned about git blame noise, I can move those lint changes to a separate commit (might require a separate PR since we squash merge?), and then add this commit to a .git-blame-ignore-revs and then it'll be ignored on git blame.

That's a good idea. Let's move all auto-formatting changes to a separate commit.