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

add .pre-commit-hooks.yaml for those using this tool #227

Closed
wasdee opened this issue Apr 7, 2020 · 16 comments
Closed

add .pre-commit-hooks.yaml for those using this tool #227

wasdee opened this issue Apr 7, 2020 · 16 comments
Labels
👀 no/external This makes more sense somewhere else

Comments

@wasdee
Copy link

wasdee commented Apr 7, 2020

https://pre-commit.com/hooks.html is missing this cool tool. 😞

Can we add the .pre-commit-hooks.yaml?

@wasdee wasdee added 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Apr 7, 2020
@ChristianMurphy
Copy link
Member

Hey @CircleOnCircles! 👋
Thanks for the suggestion!

When you say "it is missing", do you mean:

  • You'd like documentation on how pre-commit could be used to run remark-lint?
  • You'd like pre-commit added to remark-lint's repository, to run checks before committing new lint rules?
  • Something else?

@ChristianMurphy ChristianMurphy added 🙉 open/needs-info This needs some more info and removed 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Apr 7, 2020
@wasdee
Copy link
Author

wasdee commented Apr 7, 2020

You'd l#ike documentation on how pre-commit could be used to run remark-lint?

More like this one.

There are 2 jobs

  1. Update pre-commit repo that remark-lint is ready for pre-commit
  2. Add a new file to this repo or the cli repo, the yaml above. So that pre-commit know how to expose and handle this tool correctly.

@ChristianMurphy
Copy link
Member

Thanks for the clarification:
A few options:

  1. If you're up for it, you could open a PR to https://github.com/remarkjs/remark (where the CLI lives) and we could see if this would be a good fit.
  2. This could be added as a guide similar to Guides unifiedjs/unifiedjs.github.io#7, so this could be used for mdx, rehype, retext, and redot as well,
  3. pre-commit isn't the only git hooks framework, https://github.com/typicode/husky is another, husky is able to leverage CLI's provided by node_modules automatically, which can make integrating remark a lot easier.

@wasdee
Copy link
Author

wasdee commented Apr 7, 2020

Thanks for your advice. I will try to do a PR soon. I come from a Python domain, thus pre-commit seems more appealing to me.
I will definitely check out the husky.

@ChristianMurphy ChristianMurphy added 🏗 area/tools This affects tooling 🔍 status/open 🦋 type/enhancement This is great to have and removed 🙉 open/needs-info This needs some more info labels Apr 7, 2020
@wooorm
Copy link
Member

wooorm commented Aug 22, 2020

I’m all open to new integrations. Typically, they can be maintained on their own, outside of this project. In this case, as that’s a Python project and I don’t speak Python, it doesn’t sit too well with me to maintainer it here.

A couple of other integrations are listed here in the readme: https://github.com/remarkjs/remark-lint#integrations

So, if someone wants to add this, that’s welcome!

@wooorm wooorm closed this as completed Aug 22, 2020
@wooorm wooorm added 👀 no/external This makes more sense somewhere else and removed 🏗 area/tools This affects tooling 🔍 status/open 🦋 type/enhancement This is great to have labels Aug 22, 2020
@henryiii
Copy link

henryiii commented Jul 19, 2021

pre-commit is a general purpose linting framework that happens to be written in Python. It supports a host of different languages. With pre-commit.ci, there is quite a nice online tool for running the checks and updates. It's ideal if the pre-commit-hooks.yaml (which is a very small, simple file) can be in the main repository, because pre-commit caches on the revision. It has been done in a few special cases, but it either requires manually or automatically syncing tags on releases. Dropping this file into the main repo is much easier to maintain. However, on quick testing, it seems the "package.json" is missing the name/version, which are required by npm install ..

Setting this up as a local hook works, but it seems that "warnings" do not trigger a "failing" exit code, so pre-commit thinks the hooks pass.

Here's my local config:

- repo: local
  hooks:
  - id: remarklint
    name: remarklint
    language: node
    entry: remark
    types: [markdown]
    args: ["-u", "preset-lint-recommended"]
    verbose: true  # Necessary to see if this is working at all since this always "passes"
    additional_dependencies: [remark, remark-lint, remark-cli, remark-preset-lint-recommended]

PS: changing checks are even better, but I wasn't sure how to run the changing one from the CLI.

@wooorm
Copy link
Member

wooorm commented Jul 19, 2021

You can pass --frail as an arg to remark. That will result in warnings being seen as errors

@henryiii
Copy link

henryiii commented Jul 19, 2021

Thanks! I have this now:

- repo: local
  hooks:
  - id: remarklint
    name: remarklint
    language: node
    entry: remark
    types: [markdown]
    args: ["--frail", "--quiet"]
    additional_dependencies: [remark, remark-lint, remark-cli, remark-preset-lint-recommended, remark-lint-list-item-indent]

This works pretty well, though admittedly, I don't understand why I have to explicitly list remark-lint-list-item-indent to configure it in my .remarkrc file if it's included by remark-preset-lint-recommended.

To make this easier, a little repo could be made with a hook file with more-or-less the contents above, though it probably should be a node hook and the dependencies specified through package.json (so users could add ones they needed, like preset-lint-recommended or remark-lint-list-item-indent without having to specify remark and such over again). Also the versions should be pinned (at least remark-lint).

Would you be interested in keeping something like that in this org? It would need version bumps, but that should be the only maintenance needed.

@henryiii
Copy link

PS: the benefit, other than making it easier for pre-commit users to add this, and being able to be listed on https://pre-commit.com/hooks.html, would be that pre-commit.ci and pre-commit autoupdate would be able to keep this hook up-to-date like all the other non-local hooks. See https://github.com/cheshirekow/cmake-format-precommit for an example of a stand-alone repo alongside a normal repo (https://github.com/cheshirekow/cmake-format).

@ChristianMurphy
Copy link
Member

I'm not against pre-commit support for remark-lint, I'm not sure I'm for it either.
I use pre-commit myself on the occasional Python project.
There are some considerations to this integration living in the remark-lint repo which make me cautious:

  1. Would adding a YAML file to this repository offer a signifantly better experience than adding a recipe showing how to do the setup to https://unifiedjs.com/learn/ ?
  2. Most of remark-lints' maintainers don't use use pre-commit, a few use it on occasion. How would it be regularly tested to ensure it continues working?
  3. The automation in this repository is npm and github-actions based, is there a way to handle the updates to the config and do automated testing on the configs from these toolchains? (I'm not seeing it documented at https://pre-commit.com/#new-hooks and https://github.com/pre-commit/action is deprecated)
  4. Who would handle answering questions and issue triage with the integration? Would you or someone you know be interested handling the support aspect that goes along with the integration?

@henryiii
Copy link

henryiii commented Jul 20, 2021

Well, a first step might be just showing the example I have above somewhere, it does work, though it would be nice to offer an actual integration.

  1. Still not sure why everyone ties pre-commit to Python. I'm using it on a C++ project currently, and I also use it on Ruby projects.
  2. I think the correct direction is to make a remarkjs/remark-lint-pre-commit repository with a light weight package.json + .pre-commit-hooks.yaml.
  3. It should be easy to check in CI. There are a quite a few checks in the list of known hooks that check themselves.
  4. You don't need pre-commit/action; you can just add - run: pipx run pre-commit run -a on any GHA action file, no setup needed.
  5. I'd be available for a bit of help with the pre-commit side of things. I'm not very knowledgeable on the Node side, though.

I've played with setting this up, but ran into a node issue. What I'd like is to have a package.json file like this:

{
  "name": "required-but-not-used",
  "version": "0.0.0",
  "dependencies": {
    "remark-lint": "8.0.0",
    "remark": "*",
    "remark-cli": "*"
  },

And then be able to access remark somehow. But I have no idea how to run from the correct bin folder; I think it gets nested inside the requirements somewhere. Pre-commit is preparing the environment like this:

npm install --dev --prod --ignore-prepublish --no-progress --no-save
npm pack <prefix> # produces name of pkg
npm install -g <pkg>

Would you happen to know how I can add a requirement to package.json and then "export" a script from it to be used? I've tried a few things like "scripts": {"remark": "remark"}, but I can't get the "nested" requirements to show up in the "global" location in that prefix.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 20, 2021

Still not sure why everyone ties pre-commit to Python. I'm using it on a C++ project currently, and I also use it on Ruby projects

Because git hooks themselves aren't terribly complex, each language ecosystem adds abstractions around hooks tuned for that language's own tools to add value above and beyond plain hooks.

It's true that any of the above could be used with any other language, and it's perfectly fine to do so.
But each is optimized/tuned for the language and ecosystem it is written in, for pre-commit that happens to be Python.

I think the correct direction is to make a remarkjs/remark-lint-pre-commit repository with a light weight package.json + .pre-commit-hooks.yaml.

Thoughts on this @remarkjs/core and @remarkjs/maintainers?

It should be easy to check in CI. There are a quite a few checks in the list of known hooks that check themselves.

👍

You don't need pre-commit/action; you can just add - run: pipx run pre-commit run -a on any GHA action file

👍

no setup needed.

* except for this script and setting up Python

I'd be available for a bit of help with the pre-commit side of things

Thanks! 🙇

But I have no idea how to run from the correct bin folder

npx (https://docs.npmjs.com/cli/v7/commands/npx) can automatically find and run scripts in the bin folder.
Or it can be manually accessed as it will be consistently located relative to the project root in ./node_modules/.bin/remark after npm install is run

@henryiii
Copy link

Because git hooks themselves aren't terribly complex, each language ecosystem adds abstractions around hooks

pre-commit is a linter runner, more than just a githooks framework (which it also is - but you don't need to run pre-commit install, I often just use pre-commit run -a). It supports linters in many languages natively, automatically handling environment setup, etc, for each one. Ruby's overcommit is very much a hook manager; any extra language support is just basically running shell scripts. Husky also seems to be very JS focused. While pre-commit supports conda, coursier, docker, docker_image, dotnet, fail, golang, node, perl, python, python_venv, r, ruby, rust, swift, pygrep, script, or system. It's obviously strong in Python with a few repeats there, and you are promised a working version of Python when running it since it's written in Python, but otherwise, it's quite general, and you don't really have to touch Python or manual shell commands. Now that it has its own CI service, you really don't have to touch Python to use it. I use it in C++ and Ruby projects (besides, of course, Python ones).

except for this script and setting up Python

Well, pipx run pre-commit run -a is a bash script, though I'd concede that pipx is a Python thing. You do not need setup-python, however; in fact, it will do nothing. pipx is a supported stand-alone package manager on all GitHub runners (and in fact, it's used to setup up a few of the other built in tools and is used in some actions).

Thanks for the pointers, I'll tinker on this and get back to you.

@ChristianMurphy
Copy link
Member

pre-commit is a linter runner

shell is a lint runner too 😉 🙂
It supports tools in all the languages you mentioned and more.

it's quite general

Agreed, and that's part of its niche.

Many languages have comprehensive, integrated, and opinionated package management and script/test/lint runners.
Node has NPM and Webpack (among others), Java has Maven and Gradle, Python has Poetry, Ruby has Gem and Rake, etc.
Most projects either have: a single language, single package manager, and single build tool, or are a monorepo with several subprojects each with its own language, package manager, and build tool.
In that context, developers sometimes/often prefer commit hooks and linting that is tied into the tool chain they are using.

pre-commit fits in well with Python projects, or with projects that don't have an established tool chain.

I use it in C++ and Ruby projects (besides, of course, Python ones).

I don't know why you are taking this as a personal affront.
Again, it's fine to use pre-commit on projects, Python or not, it's a wonderful tool. ❤️

There are also other wonderful tools, the tools listed above #227 (comment), not to mention SonarQube, and host of other code quality tools.
Each have their own niche, pre-commit's is primarily git hooks for Python.

@muppetjones
Copy link

Came looking for this integration specifically. Without it, I'll probably just fall back to prettier. I wouldn't say it's critical, but markdown is annoying enough to deal with without having to deal with linter compatibility. This is personal preference, of course.

@ChristianMurphy
Copy link
Member

@muppetjones it helps to read discussion before commenting. 🙂
In particular see #227 (comment) and #227 (comment)

If you want to get started quickly:

- repo: local
  hooks:
  - id: remarklint
    name: remarklint
    language: node
    entry: remark
    types: [markdown]
    args: ["--frail", "--quiet"]
    additional_dependencies: [remark, remark-lint, remark-cli, remark-preset-lint-recommended]

If you'd like to build a mirror with something more specific/custom, feel free to https://github.com/pre-commit/pre-commit-mirror-maker and that can be shared back with the community an integration linked in the documentation.

I'm locking this issue as this is not a planned code change in this repo, and the issue tracker is a place to track changes for this repository specific, rather than questions or discussions.

If folks have questions on using remark-lint the Q&A discussion board is open. https://github.com/orgs/remarkjs/discussions

@remarkjs remarkjs locked as resolved and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
👀 no/external This makes more sense somewhere else
Development

No branches or pull requests

5 participants