RichardLitt / standard-readme-cli

A CLI for standard readme
MIT License
2 stars 1 forks source link

Add pre commit hook #6

Closed tpansino closed 3 years ago

tpansino commented 3 years ago

Pull Request

Migrated from https://github.com/RichardLitt/standard-readme-preset/pull/40

Description of Changes

RichardLitt commented 3 years ago

I don't like pre-commit hooks, because they seem to me to make changes that the developer doesn't know about after they think they are done. I think the UX is weird.

Instead, this should be a CI check, I think.

What am I missing in this view?

tpansino commented 3 years ago

@RichardLitt I only added a .pre-commit-hooks.yaml, which defines a hook so that other repos (like mine) can download and use this CLI as a pre-commit check. I didn't change the UX for this repo.

I agree that checks should also be done in CI. That's why my repos use this GitHub Action as well. It allows for both workflows - the users get faster feedback and don't push up broken commits, and the commits are still checked via CI in case the user doesn't want/forgets to install pre-commit hooks before pushing.

I like giving people options. 😄

RichardLitt commented 3 years ago

@RichardLitt I only added a .pre-commit-hooks.yaml, which defines a hook so that other repos (like mine) can download and use this CLI as a pre-commit check. I didn't change the UX for this repo.

If this is true, does this file belong in this repo? Can't you implement it on your end, or could we include it in another npm package?

tpansino commented 3 years ago

If this is true, does this file belong in this repo?

Yes.

Pre-commit has 2 parts - producers of hooks declare a .pre-commit-hooks.yaml file, and consumers use them via .pre-commit-config.yaml file. Pre-commit then git clones each repo in the config and executes hooks based on the settings it finds the hooks file. See the docs for more details.

Can't you implement it on your end

No, it requires 2 halves by design as described above.

could we include it in another npm package?

Such a package would have to just wrap and install this package. I don't think that's valuable enough to you justify the maintenance cost of one more repo. 🙂


My goal here is to provide an additional way for people to easily discover and use this tool. I'm personally using the hook via my fork to check READMEs on some Python repos I maintain. Without a hook I would have to add a package.json to my Python repo, install this CLI via npm, and either remember to run it locally, or put it in the CI pipeline.

With the hook, I only need:

> cat .pre-commit-config.yaml
repos:
  - repo: git://github.com/tpansino/standard-readme-cli
    rev: 422e367
    hooks:
      - id: standard-readme

And now each time I commit, pre-commit will run and stop me if I've made a change that is out of spec. In addition I can use the GitHub Action I linked earlier to ensure the spec is met as part of the CI pipeline (in case someone doesn't want to install pre-commit or the hooks)

For what it's worth, there is a registry of pre-commit hooks. I'm planning to add this repo after this PR is merged, which will hopefully increase visibility, as I think this is an awesome spec and lint tool and I want more people to know about it. 👍

RichardLitt commented 3 years ago

Got it. Thank you. :)

tpansino commented 3 years ago

No, thank you! I appreciate the pushback. 👍