DavidAnson / markdownlint-cli2

A fast, flexible, configuration-based command-line interface for linting Markdown/CommonMark files with the markdownlint library
MIT License
341 stars 46 forks source link

Output format of pre-commit hooks very messy #301

Closed skwde closed 4 months ago

skwde commented 5 months ago

This is bascially a followup of #216 .

I have the .markdownlint-cli2.yaml

noProgress: false

# Fix any fixable errors
fix: true

outputFormatters:
  -
    - markdownlint-cli2-formatter-pretty
    - appendLink: true
    #- markdownlint-cli2-formatter-summarize
    #- byFileByRule: true

and the .pre-commit-config.yaml

  - repo: https://github.com/DavidAnson/markdownlint-cli2
    rev: v0.13.0
    hooks:
      - id: markdownlint-cli2
        additional_dependencies:
          - markdownlint-cli2-formatter-pretty
          #- markdownlint-cli2-formatter-summarize

I also tried markdownlint-cli2-formatter-summarize but the problem stays the same.

The output is super messy, basically it is reported as blocks. When there are errors, the blocks look like

markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: <some files, probably the oncs checked>
Linting: 37 file(s)
Summary: 1 error(s)
<file and line> <error with link>

and one basically has to find them between numerous blocks of

markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: <some files, probably the oncs checked>
Linting: 28 file(s)
Summary: 0 error(s)

Note, this problem only persists when running via pre-commit and not when doing something like markdownlint-cli2 "**/*.md". In that case the output is neat and nice

markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: **/*.md
Linting: 584 file(s)
Summary: 1 error(s)
<file and line> <error with link>

So it looks like the problem is that pre-commit calls markdownlint-cli2 in batches with individual filenames rather than **/*.md.

How do I get markdownlint-cli2 pre-commit output also that neat and nice?

DavidAnson commented 5 months ago

As you say, the issue here is pre-commit behavior. I do not use it, but it sounds like maybe you have multiple hooks or are scanning multiple commits at once. Maybe you can combine everything and use pre-commit run --all-files?

https://pre-commit.com/index.html

skwde commented 5 months ago

That is exactly what I do.

skwde commented 5 months ago

The related issue on pre-commit was closed with

doesn't seem like a pre-commit problem

see https://github.com/pre-commit/pre-commit/issues/3179

DavidAnson commented 5 months ago

As you point out above, the issue occurs because the markdownlint-cli2 tool is being invoked multiple times instead of just once. If the pre-commit maintainers say that is how pre-commit is supposed to behave, I defer to them because they know pre-commit better. (Although I do think it would be better for pre-commit to invoke a tool just once.)

DavidAnson commented 5 months ago

Based on their follow-up comment, it looks like a tool might be able to opt out of parallel execution thusly: https://pre-commit.com/#hooks-require_serial

I can look into adding that.

skwde commented 5 months ago

Ok, perfect. Thanks! I just tested setting require_serial: true and it works.

It now only shows a single block, however this single block also becomes huge due to the Finding: section mentioning all files markdownlint-cli2 was called with.

DavidAnson commented 5 months ago

Great! The option noProgress can be set to true in the root .markdownlint-cli2.jsonc file to hide the "Finding:" message.

skwde commented 5 months ago

Works like a charm! Thanks.

Maybe your provided pre-commit hook should contain these two settings by default?

DavidAnson commented 5 months ago

noProgress can't be set in the hook, only the project's options file. require_serial could be customized by the hook, but I try to avoid overriding defaults. There may be scenarios where it's useful to parallelize this task? My preference is to leave things as they are now, BUT I'll add a note to the README about this later tonight or tomorrow. If you don't mind, can you please paste the .pre-commit-config.yaml that you ended up with?

skwde commented 5 months ago

Ahh you are right. I thought one could use the pre-commit's args (https://pre-commit.com/#config-args) to set noProgress: true for markdownlint-cli2. It seems however that markdownlint-cli2 only accepts an entire conifg file, not individual options.

For now (<1000 files) I didn't see a real difference between parallel / serial execution, but it might be handy later on.

Switching back to require_serial: false (the default) with noProgress: true then gives again output for every execution of markdownlint-cli2 like

markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
index.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## heading"] https://github.com/DavidAnson/markdownlint/blob/v0.33.0/doc/md041.md
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)
markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)

So maybe it is possible to also suppress markdownlint-cli2 v0.12.1 (markdownlint v0.33.0)?

In any case here is the require_serial: true .pre-commit-config.yaml

---
repos:
  - repo: https://github.com/DavidAnson/markdownlint-cli2
    rev: v0.12.1
    hooks:
      - id: markdownlint-cli2
        # ## https://github.com/DavidAnson/markdownlint-cli2/issues/301
        require_serial: true
        additional_dependencies:
          - markdownlint-cli2-formatter-pretty
          # - markdownlint-cli2-formatter-summarize
DavidAnson commented 5 months ago

Setting the new noBanner configuration file option to true will hide the banner line.

skwde commented 5 months ago

Perfect, then require_serial: true is actually not needed. Just set

noProgress: true
noBanner: true

in .markdownlint-cli2.yaml.

Aside, it seems that the VSCode extension uses the wrong config yaml schema?! I use v0.54.0 which I guess is the latest. See following screenshot.

Screenshot 2024-04-16 at 13 30 39
DavidAnson commented 5 months ago

The VS Code extension does not have the latest library/CLI2 version yet.