Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing pre-commit and markdownlint-cli2 #701

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ns-rse
Copy link
Contributor

@ns-rse ns-rse commented Jul 22, 2023

Closes #700

Adds pre-commit to the repository (and configures but doesn't yet enable pre-commit.ci). Some basic pre-commit hooks are included but the meat of the linting is done via markdownlint-cli2 which is configured via .markdownlint-cli2.yaml.

Retrospective linting of the .md and .sh files included in this PR has been undertaken (mostly line-length issues, but a typo has been corrected in _posts/2022-12-22-git-blame.md too).

README.md has a new section on setting up and using pre-commit for this work and includes details of why retrospective linting has not been applied more widely (basically I didn't want to break any pages) and how to ignore revisions should anyone wish to undertake this task.

markdownlint-cli2 does have an option to fix: true which will where possible fix things automatically. I had a go at doing this and fixing all existing code with pre-commit run --all-files but found it wasn't applying things very well (e.g. line lengths weren't being corrected[1]) and I was very wary of messing up the site.

One thing to discuss would be whether we enable pre-commit.ci. I didn't want to foist this upon people without consensus being reached.

[1] This led me to writing a short script (wrap) that uses fmt to sensibly wrap lines for all files of a particular type at a specified length (and in parallel too) if anyone wanted to have a go at sorting things out.

Closes #700

Adds [pre-commit](https://pre-commit.com) to the repository (and configures but doesn't yet enable
[pre-commit.ci](https://pre-commit.ci)). Some basic `pre-commit` hooks are included but the meat of the linting is done
via [markdownlint-cli2](https://github.com/DavidAnson/markdownlint-cli2) which is configured via
`.markdownlint-cli2.yaml`.

Retrospective linting of the `.md` and `.sh` files included in this PR has been undertaken (mostly line-length issues,
but a typo has been corrected in `_posts/2022-12-22-git-blame.md` too).

`README.md` has a new section on setting up and using `pre-commit` for this work and includes details of why
retrospective linting has not been applied more widely (basically I didn't want to break any pages) and how to ignore
revisions should anyone wish to undertake this task.

`markdownlint-cli2` does have an option to `fix: true` which will where possible fix things automatically, but I had a
go at doing this and it wasn't applying things very well (e.g. line lengths weren't being sorted). I have written a
short script ([`wrap`](https://gitlab.com/nshephard/dotfiles/-/blob/master/bin/wrap)) that uses `fmt` to sensibly wrap
lines for all files of a particular type at a specified length (and in parallel too) if anyone wanted to have a go at
sorting things out.

One thing to discuss would be whether we enable [pre-commit.ci](https://pre-commit.ci/). I didn't want to foist this
upon people without consensus being reached.
@ns-rse ns-rse added the CI Continuous Integration and Development label Jul 22, 2023
@ns-rse ns-rse requested a review from a team July 22, 2023 10:43
@Robadob
Copy link
Member

Robadob commented Jul 24, 2023

This table is written in html with custom CSS because the website's theme does not show table borders. Should be corrected tbh.

_posts/2016-09-12-intel_R_iceberg.md:64:1 MD033/no-inline-html Inline HTML [Element: style]
_posts/2016-09-12-intel_R_iceberg.md:70:1 MD033/no-inline-html Inline HTML [Element: table]
_posts/2016-09-12-intel_R_iceberg.md:71:3 MD033/no-inline-html Inline HTML [Element: tr]
...

@Robadob
Copy link
Member

Robadob commented Jul 24, 2023

Not sure I agree with this rule.

MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "Joe Heffer, Research Data Engi..."]

image

Robadob added 3 commits July 24, 2023 14:27
One of the external images linked by Hacktoberfest 2018 was 404, so replace with image from the bottom of the page and removed the bottom image.

Resolved missing alt-text in assets dir, but this directory should probably be ignored.
These are 3rd party.
Most cases where console output, so marked as text, others I attempted to specify the most suitable language.
@ns-rse
Copy link
Contributor Author

ns-rse commented Jul 24, 2023

Could it not be re-written as

## Type Hinting in Python - Joe Heffer (Research Data Engineer, IT Services)

...which makes more sense to me since its...

## <Title> - <Author> (<Position>)

...and doesn't require another level of heading.

@ns-rse
Copy link
Contributor Author

ns-rse commented Oct 3, 2023

Shall we merge this and use the linting going forward and leave the older posts as is?

@Robadob
Copy link
Member

Robadob commented Oct 3, 2023

🤷‍♂️

@ns-rse
Copy link
Contributor Author

ns-rse commented Oct 4, 2023

It doesn't require that old code be linted unless a file is touched/modified and included in a commit since pre-commit only looks at the files included in commits (in the background it makes a stash and then undes it after running against the included files).

It would of course require contributors to have pre-commit installed locally though (no need to install the markdownlint-cli2 since pre-commit takes care of that by downloading an environment). Fortunately someone wrote a useful blog post on pre-commit 😁

@ns-rse ns-rse mentioned this pull request Sep 9, 2024
@ns-rse
Copy link
Contributor Author

ns-rse commented Sep 11, 2024

Any love for this? Should I spend time resolving the conflicts that have arisen?

I guess the main question is whether people would use the hooks locally (which provide a much quicker feedback loop).

@ns-rse ns-rse mentioned this pull request Oct 7, 2024
ns-rse added a commit that referenced this pull request Dec 11, 2024
Closes #852

More than just those changes though as I still have [pre-commit](https://pre-commit.com) configured and so linted the
page with [Markdownlint-cli2](https://github.com/DavidAnson/markdownlint-cli2) (see #701).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration and Development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint Markdown with pre-commit hook
2 participants