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

Feature/pre commit hook #321

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JensHeinrich
Copy link

Small wrapper to use alex as a pre-commit hook

@wooorm
Copy link
Member

wooorm commented Sep 13, 2021

What does it do? For who is this?

@JensHeinrich
Copy link
Author

What does it do?

It's a wrapper to use alex inside of non javascript projects

For who is this?

For git users writing texts

@wooorm
Copy link
Member

wooorm commented Sep 18, 2021

  • How should people use this? There are no docs.
  • How can we test this? There are no tests.
  • Why is it needed to have this code in this repo? Could it be somewhere else?

@JensHeinrich
Copy link
Author

* How should people use this? There are no docs.

Will add an example on how to enable it.
Basically pre-commit needs to be installed on the system via pip install pre-commit and the repository specific hooks are "installed" (enabled) for the repository by executing pre-commit install in the repository.
Afterwards the in the pre-commit-config specified version of alex will be run on every commit against all text files inside the repository.
If alex returns a failing state (e.g. not a 0) the commit will be aborted and the user will be informed what errrors were detected.
An example config for running alex on a repo on every commit is already included, but I will document it.

* How can we test this? There are no tests.

Basically just run pre-commit run alex --all-files on the alex-repo itself or have the hooks themselves enabled locally.

* Why is it needed to have this code in this repo? Could it be somewhere else?

It seems to me to be the cleanest way to bring the configuration for running alex in the alex-repo as is done for ansible-lint and many more
Technically it could be moved to another repository, like done with eslint.

@JensHeinrich
Copy link
Author

Ideally the pre-commit-config.yaml would be extended to include the suggested linters and formatters for the alex-repo itself to make most use of it.
Those linters and formatters would then be run (after enabling it) on every commit and ensure a more consistent codestyle (and help contributers by automating the testing to make human errors less frequent)

@adam-moss
Copy link

Just wondering if this is likely to be merged?

@jussihi
Copy link

jussihi commented Jun 15, 2023

@wooorm Will this be merged anytime soon?
pre-commit is very well known framework for managing git pre-commit hooks. I'm currently working on a project that would require alex.js to be pre-commit "compatible". I don't know it this repo itself should have the .pre-commit-config.yaml file - but alex.js definitely should be able to be used as a pre-commit hook for any git repo (include the .pre-commit-hook.yaml).

Not having .pre-commit-hook.yaml in this repo would just not let alex.js show its full potential. With the hook file in place it will be so much easier to plug and play this checker into any existing git repo with just a couple lines of configuration, and I think that alex.js's usage would increase drastically.

I could create a cleaned-up version of this pull request (i.e. without the .pre-commit-config.yaml) but I think that @JensHeinrich would prefer to clean up their request and have their contribution added instead :-)

@wooorm
Copy link
Member

wooorm commented Jun 15, 2023

There are open issues with this PR. Notably docs and examples. But it’s still in the discussion phase anyway: needs more convincing for why this is needed here. How should this be maintained? Why does it need to be here? It looks like pre-commit users can also place pre-commit yaml in their own repos? Looking at https://pre-commit.com/hooks.html, many many pre-commit hooks are in their own repos?

With the hook file in place it will be so much easier to plug and play this checker into any existing git repo with just a couple lines of configuration, and I think that alex.js's usage would increase drastically.

I don‘t understand this. pre-commit has 10k stars, it’s pretty popular but not terribly so. It’s a Python-world thing. alex is a CLI that can be used from anywhere already: in a Makefile, in npm scripts. It can already be used in every git repo?

@adam-moss
Copy link

adam-moss commented Jun 15, 2023

There are open issues with this PR. Notably docs and examples.

Agreed

But it’s still in the discussion phase anyway: needs more convincing for why this is needed here. How should this be maintained? Why does it need to be here?

This is the authorative source for Alex, ergo it is preferable to sit within this repo.

Maintenance would be ~nil unless wholesale changes were made to the cli itself.

It looks like pre-commit users can also place pre-commit yaml in their own repos? Looking at https://pre-commit.com/hooks.html, many many pre-commit hooks are in their own repos?

I think you may be mixing up pre-commit-config.yaml, which sits in a user repo, and pre-commit-hooks.yaml, which sits in the tooling repo.

In essence pre-commit-hooks defines what should be installed/run/how and pre-commit-config defines what the user wants to include.

With the hook file in place it will be so much easier to plug and play this checker into any existing git repo with just a couple lines of configuration, and I think that alex.js's usage would increase drastically.

Totally this.

I don‘t understand this. pre-commit has 10k stars, it’s pretty popular but not terribly so. It’s a Python-world thing. alex is a CLI that can be used from anywhere already: in a Makefile, in npm scripts. It can already be used in every git repo?

It's not a python world thing. we have pre-commit in ~11k repos of which < 1% are python. Most are java or node.

Pre-commit is more akin to a cross platform/language/ecosystem alternative to nodes husky.

@jussihi
Copy link

jussihi commented Jun 15, 2023

I couldn't push changes to this branch (obviously) since I don't have rights to @JensHeinrich 's branch. So I've created my own fork and feature branch, which you can find here: https://github.com/jussihi/alex/tree/feature-pre-commit. I used @JensHeinrich's commits as a base for my branch. I cherry-picked 3 first commits - I've omitted the .pre-commit-config.yaml from there since running pre-commit inside alex repo itself would be nonsense - it is already done via some node-based test system.
Is it enough to have this readme entry or should a longer one be written? I'm also considering changing the hook description to something shorter, maybe "Catch insensitive, inconsiderate writing." from alex's readme would suit here? If you are okay with this, I will open a new PR for my changes.

@wooorm
Copy link
Member

wooorm commented Jun 15, 2023

How should this be maintained

There’s a potential problem with a single person maintaining this for years in the past and in the future, a drive-by commit from someone who likes pre-commits and then potentially leaving for the woods: I can’t maintain code I don’t use. I can only remove this 3 years down the road.

I think you may be mixing up pre-commit-config.yaml, which sits in a user repo, and pre-commit-hooks.yaml, which sits in the tooling repo.

So: users have their own .pre-commit-config.yaml in their repo, right? And the proposal is to add a .pre-commit-hooks.yaml in get-alex/alex, right? My Q is: why don’t users copy/paste the 9 lines .pre-commit-hooks.yaml into their own repo? This seems like a good solution to me for users who want their own Git hooks integration?

We can add this in the readme here?

Looking at pre-commit.com/hooks.html, many many pre-commit hooks are in their own repos?

This was a separate point from the earlier: I meant to say that I see many “wrapper” repos in the list: https://pre-commit.com/hooks.html. So many tools don’t include pre-commit-hooks themselves.

@jussihi
Copy link

jussihi commented Jun 15, 2023

How should this be maintained

There’s a potential problem with a single person maintaining this for years in the past and in the future, a drive-by commit from someone who likes pre-commits and then potentially leaving for the woods: I can’t maintain code I don’t use. I can only remove this 3 years down the road.

I completely understand this. I cannot guarantee that I would be fixing this in the future if something breaks, but it seems like there are more people interested in having this added to pre-commit than just me (see this thread). Also, once more people would use it with pre-commit, the "integration" would definitely be well maintained.

I think you may be mixing up pre-commit-config.yaml, which sits in a user repo, and pre-commit-hooks.yaml, which sits in the tooling repo.

So: users have their own .pre-commit-config.yaml in their repo, right? And the proposal is to add a .pre-commit-hooks.yaml in get-alex/alex, right?

Exactly.

My Q is: why don’t users copy/paste the 9 lines .pre-commit-hooks.yaml into their own repo? This seems like a good solution to me for users who want their own Git hooks integration?

Because pre-commit doesn't work that way. Or yes, it does if you want to, but then you would have to have a local copy of alex and locally add this .pre-commit-hooks.yaml into alex's repo and commit it. So better say it's not meant to be used that way.

You can think pre-commit as sort of a package manager. You give it a tool's git repository location, and a tag/commit hash. pre-commit then clones the repo and works with the instructions it finds from the tool repo's .pre-commit-hooks.yaml and installs this tool to your git commit hooks.

We can add this in the readme here?

Yes, I created my own copy of this feature branch, should I create a PR of it?

Looking at pre-commit.com/hooks.html, many many pre-commit hooks are in their own repos?

This was a separate point from the earlier: I meant to say that I see many “wrapper” repos in the list: https://pre-commit.com/hooks.html. So many tools don’t include pre-commit-hooks themselves.

This is another way to do it. We just need to hope that the pre-commit team/org/whatever agrees to create this "mirror-alex" repo and start tracking your repo from there. You can check how these mirror repos work - they create a mock .pre-commit-hooks.yaml file and just add the parent tools as requirements to them.

Obviously there are pros and cons in both ways of doing this, feel free to choose what are pros and what are cons:

  1. Storing .pre-commit-hooks.yaml in tool's own repo:
  • Simpler, tool repo only requires .pre-commit-hooks.yaml
  • More to maintain for the tool's owner. However, "more" here is ~nil after initial addition as earlier pointed out by @adam-moss
  1. Storing .pre-commit-hooks.yaml inside a mirror repo
  • More complex, see any mirror repo how it's done. (However not your problem :-) )
  • There's no guarantee that alex would be added to mirror repos, I don't know who chooses which tools are added as mirror repos to pre-commit's github org.
  • Tool repo owner/maintainer might be completely unaware that their tool is pre-commit compatible. This might be a good or a bad thing - obviously it would at least mean a tiny amount of less maintenance

@adam-moss
Copy link

There’s a potential problem with a single person maintaining this for years in the past and in the future, a drive-by commit from someone who likes pre-commits and then potentially leaving for the woods: I can’t maintain code I don’t use. I can only remove this 3 years down the road.

Totally understand, that is the perennial issue with open source and as the maintainer it is absolutely the right challenge to make.

From hooks I maintain, I've only had to edit the pre-commit-hook.yaml file when:

  1. I've changed the language of the hook
  2. I've changed the cli interface

So: users have their own .pre-commit-config.yaml in their repo, right? And the proposal is to add a .pre-commit-hooks.yaml in get-alex/alex, right? My Q is: why don’t users copy/paste the 9 lines .pre-commit-hooks.yaml into their own repo? This seems like a good solution to me for users who want their own Git hooks integration?

To put it in their own config its a little different:

    - repo: local # Replace if https://github.com/get-alex/alex/pull/321 is merged
      hooks:
          - id: alex-js
            name: Alex.js
            description: Find gender favoring, polarizing, race related, religion inconsiderate, or other unequal phrasing in text.
            entry: alex
            language: node
            types:
                - markdown
            additional_dependencies:
                - alex

The downside of this approach is local hooks don't get updated, so when you release a new version the end user would never receive it.

We can add this in the readme here?

Certainly could, or, if you'd prefer, we could create a "mirror" repo like e.g. https://github.com/pre-commit/mirrors-prettier which would remove any potential burden from yourself / this project?

I'd certainly be happy to do that 👍

@JensHeinrich
Copy link
Author

The biggest advantage of keeping it here, is any new tag / version of alex can be easily upgraded too by just running pre-commit autoupdate

@JensHeinrich
Copy link
Author

Also thanks to @jussihi and @adam-moss for better explaining the why

@mfisher87
Copy link

I made a mirror repo: https://github.com/mfisher87/alexjs-pre-commit-mirror

This is my first time doing so, would appreciate any collaboration :)

JensHeinrich added a commit to JensHeinrich/alexjs-pre-commit-mirror that referenced this pull request May 3, 2024
Add description as used in get-alex/alex#321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants