Lints existing code base with pre-commit & markdownlint-cli2

RSEToolkit / rse-competencies-toolkit

A toolkit to define the skills, competencies and diverse progression pathways for RSEs to help track and manage their professional profiles and development.

https://rsetoolkit.github.io/rse-competencies-toolkit/

Other

37 stars 1 forks source link

Lints existing code base with pre-commit & markdownlint-cli2 #55

Closed ns-rse closed 1 year ago

ns-rse commented 1 year ago

Closes #54

Lints existing code base with basic pre-commit rules and markdownlint-cli2 pre-commit hook.

Aside from about.md and LICENSE.md most changes are courtesy of the end-of-file-fixer.

In addition...

Adds ignore rule for svg files to .markdownlint-cli2.yaml configuration.
Introduces .git-blame-ignore-revs file to ignore commits associated with linting (including this one via git commit -a). For more on this file see Who's to Blame?.
Adds Emacs temporary files to .gitignore.

ns-rse commented 1 year ago

Hmm, not sure why the *.svg files were linted given the glob rule for includes and the explicit exclusions. :thinking:

I'll investigate and resolve.

davehorsfall commented 1 year ago

@ns-rse, just wanted to acknowledge this PR. Thanks for creating and will review in the next couple of days. I was bogged down last week.

ns-rse commented 1 year ago

Thanks @davehorsfall and not a problem at all (I love pre-commit and think its a great tool to use). I've similarly been busy but will see if I can work out why the glob/ignores aren't being respected correctly by markdownlint-cli2 soon.

davehorsfall commented 1 year ago

@ns-rse I also wanted to ask about the pre-commit ci plugin that has been activated on the repo. If a contributor commits code that fails the linting checks, what happens? Will it be rejected, or will it just notify and attempt to fix, etc? I just wanted to understand if this presents a technical barrier to people who don't adopt pre-commit, but want to contribute. Thanks.

ns-rse commented 1 year ago

I'll take a look at the .svg issue after work and try and suss it out.

With regards to pre-commit.ci and contributions it depends on how the repository is set up and what the errors are.

If markdownlint-cli2 can correct the errors it will do so and pre-commit.ci will automatically make add a commit to the PR so these are incorporated in the pull request.

If they can't be corrected then it depends on how the branch protection rules are configured for main and whether the Require status checks to pass before merging option is selected.

Broadly I'd encourage adoption of pre-commit since it helps push capturing problems further "left"(/earlier) in the development pipeline/cycle. I realise however not everyone may be willing to do so, in which case there are two options that I can think of...

Merge text/code that doesn't pass the linting hooks that are in place.
Someone else resolves them on the contributors behalf and adds the commits to the PR.

davehorsfall commented 1 year ago

Thanks for clarifying. It's OK for the pre-commit setup to mandate that errors are fixed before a PR is merged. This can be managed during the review stage, and allows us to maintain good code quality.

I was just concerned that contributions from people might be completely rejected if they didn't pass checks, which would significantly decrease the accessibility of the project.

ns-rse commented 1 year ago

Ok, I'm a fool and wasn't reading the output carefully enough as has been kindly pointed out to me. Its not markdownlint-cli2 that is doing the linting here, rather its the end-of-file-fixer from the earlier pre-commit-hooks. Looking at the affected files the change doesn't appear to have had any impact on their rendering so I guess there is no harm.

This is perhaps ready for merging.

davehorsfall commented 1 year ago

Its always obvious with hindsight! :smile:

You're right, the final new line in the svg doesn't impact rendering.