bemanproject / exemplar

Example Beman project
Other
11 stars 16 forks source link

Introduce basic linting infrastructure #34

Closed wusatosi closed 1 month ago

wusatosi commented 2 months ago

Introduce pre-commit based linting infrastructure:

Action Items:

Reference implementation: https://github.com/beman-project/optional26/pull/52

Closes: #29 .

wusatosi commented 1 month ago

Edit: switch to https://github.com/igorshubovych/markdownlint-cli , below report is out of date.

markdownlint doesn't provide a auto-format functionality, which would be annoying.

Initial error report:

README.md:89: MD007 Unordered list indentation
README.md:13: MD012 Multiple consecutive blank lines
README.md:22: MD012 Multiple consecutive blank lines
README.md:194: MD012 Multiple consecutive blank lines
README.md:9: MD013 Line length
README.md:16: MD013 Line length
README.md:20: MD013 Line length
README.md:35: MD013 Line length
README.md:134: MD013 Line length
README.md:234: MD013 Line length
README.md:248: MD013 Line length
README.md:270: MD013 Line length
README.md:276: MD013 Line length
README.md:289: MD013 Line length
README.md:80: MD022 Headers should be surrounded by blank lines
README.md:116: MD031 Fenced code blocks should be surrounded by blank lines
README.md:95: MD033 Inline HTML
README.md:96: MD033 Inline HTML
README.md:112: MD033 Inline HTML
README.md:113: MD033 Inline HTML
README.md:122: MD033 Inline HTML
README.md:123: MD033 Inline HTML
README.md:143: MD033 Inline HTML
README.md:144: MD033 Inline HTML
README.md:203: MD033 Inline HTML
README.md:204: MD033 Inline HTML
README.md:231: MD033 Inline HTML
README.md:232: MD033 Inline HTML
README.md:244: MD033 Inline HTML
README.md:245: MD033 Inline HTML
README.md:265: MD033 Inline HTML
README.md:266: MD033 Inline HTML
README.md:284: MD033 Inline HTML
README.md:285: MD033 Inline HTML
README.md:234: MD034 Bare URL used

I looked over all the report, we need to at least omit MD033 Inline HTML and MD034 Bare URL used.

For MD033, we need <details> and <summary> to provide fold-able snippets.

For MD034, I think the linter is having trouble parsing markdown inside of <details>.

https://github.com/beman-project/exemplar/blob/0ac15db802577211f477a707ea6c0ce3acfc2075/README.md?plain=1#L234

steve-downey commented 1 month ago

For CI I have a strong preference for not having the linter make changes. Most of them make mistakes. clang-format is a rare exception, and even it can produce nonsense on language forms it doesn't understand yet.

wusatosi commented 1 month ago

Markdown error report (in addition to --disable=MD033, which flags use of <details>):

README.md:9:81 MD013/line-length Line length [Expected: 80; Actual: 296]
README.md:13 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
README.md:16:81 MD013/line-length Line length [Expected: 80; Actual: 208]
README.md:20:81 MD013/line-length Line length [Expected: 80; Actual: 114]
README.md:22 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
README.md:35:81 MD013/line-length Line length [Expected: 80; Actual: 100]
README.md:134:81 MD013/line-length Line length [Expected: 80; Actual: 206]
README.md:234:81 MD013/line-length Line length [Expected: 80; Actual: 202]
README.md:248:81 MD013/line-length Line length [Expected: 80; Actual: 127]
README.md:270:81 MD013/line-length Line length [Expected: 80; Actual: 126]
README.md:276:81 MD013/line-length Line length [Expected: 80; Actual: 165]
README.md:2[89](https://github.com/beman-project/exemplar/actions/runs/11149346248/job/30988022469?pr=34#step:4:94):81 MD013/line-length Line length [Expected: 80; Actual: 204]

This looks reasonable to me.

Edit: updated MD013 to 120 to conform with .clang-format.

wusatosi commented 1 month ago

For CI I have a strong preference for not having the linter make changes. Most of them make mistakes. clang-format is a rare exception, and even it can produce nonsense on language forms it doesn't understand yet.

Current CI does not push format changes to pr/ main branch, I would be interested in putting auto clang-format in but that's out of the scope of this PR.

wusatosi commented 1 month ago

Besides updating README, all configuration should be done. Opening for comments and reviews.

wusatosi commented 1 month ago

Needs to include: https://github.com/pre-commit/action/issues/7#issuecomment-1016529534

And merge main.

wusatosi commented 1 month ago

Weird, pr actions are not triggered anymore. GitHub must be down or smt

GitHub was being GitHub...

wusatosi commented 1 month ago

I don't think pre-commit's action's caching is working. I have never seen it reuse the previous environment (always cache-miss). I will keep an eye on this after the pr is merged and file an issue upstream later if needed.

wusatosi commented 1 month ago

@camio This pr is ready to merge now, I don't want to block progress of #46 as we have overlapping config file.

I can file a separate pr to add documentation.