Feature/add pre commit hook

[wbekerom]

Hi @florianschanda,

As discussed here: #152

This will implement mh-style and mh-metric hooks for the pre-commit package. Pre-commit requires pip install . to work, i.e. setup.py to exist. I changed this and update the makefile such that make package still works.

If you agree on the changes I'll update the docs.

[florianschanda]

I am new to https://pre-commit.com, so I need some help understanding :)

As far as I can work out, the way they do python hooks is very weird. They seem to:

• clone from a repo
• build + install locally
• then run that

Hence the surprising requirement for being able to install using pip install .?

But this will not work with MH; I make no guarantee that the master branch is stable. So as a user you need to bake in a specific revision in your hook; there is no way to get the latest stable version. And then you need to manually update, which is not nice either.

I would have imagined their python hooks use a named PyPi package; install that and run it. But there seems to be no support for doing that.

Maybe a better approach is: https://pre-commit.com/#system, or perhaps https://pre-commit.com/#script; what do you think?

Or indeed a bug/feature report for pre-commit, to ask for a better way to run python hooks.

[florianschanda]

In addition, you might want to also use mh_lint, and that is not in setup_gpl.py; that is part of setup_agpl.py

I admit my packaging is a bit confusing; but maybe I just make a single AGPL package that contains both the GPL and AGPL stuff together.

It would simplify this specific problem; but I still think that pre-commit has a fundamentally surprising way of running python hooks.

[florianschanda]

I've raised https://github.com/pre-commit/pre-commit/issues/1705

[florianschanda]

@wbekerom I found a different way of running the hooks that would require no change here, only a documentation change. Can you try the following pre-commit config in your project?

repos:
-   repo: local
    hooks:
    -   id: mh_style
        name: mh_style
        entry: mh_style --process-slx
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_metric
        name: mh_metric
        entry: mh_metric --ci
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_lint
        name: mh_lint
        entry: mh_lint
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit]

If it works, I'll do the doc update and close this PR.

[wbekerom]

@wbekerom I found a different way of running the hooks that would require no change here, only a documentation change. Can you try the following pre-commit config in your project?

repos:
-   repo: local
    hooks:
    -   id: mh_style
        name: mh_style
        entry: mh_style --process-slx
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_metric
        name: mh_metric
        entry: mh_metric --ci
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_lint
        name: mh_lint
        entry: mh_lint
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit]

If it works, I'll do the doc update and close this PR.

Thanks for aligning with the pre-commit project, this works like a charm for me!

When adding the docs may I suggest to include the args field for the arguments rather than appending it to the entry point? For my use-case it would look like this:

-   repo: local
    hooks:
    -   id: mh_style
        name: mh_style
        entry: mh_style
        args: [--process-slx, --fix]
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]

[florianschanda]

OK, so looks like we have a plan + working solution. I'll update the docs tomorrow. Thanks again for the idea!

[florianschanda]

OK, this is now documented here: https://florianschanda.github.io/miss_hit/configuration.html#pre-commit

[asottile]

@wbekerom I found a different way of running the hooks that would require no change here, only a documentation change. Can you try the following pre-commit config in your project?

repos:
-   repo: local
    hooks:
    -   id: mh_style
        name: mh_style
        entry: mh_style --process-slx
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_metric
        name: mh_metric
        entry: mh_metric --ci
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit_core]
    -   id: mh_lint
        name: mh_lint
        entry: mh_lint
        files: ^(.*\.(m|slx))$
        language: python
        additional_dependencies: [miss_hit]

If it works, I'll do the doc update and close this PR.

Thanks for aligning with the pre-commit project, this works like a charm for me!

to be transparent, this is the escape hatch and not the supported way for tools to work with the framework -- there are quite a few downsides to this approach as laid out in the linked issue

ideally this package would act more like a normal python library which is installable from pip install .

[florianschanda]

Right, so the problem I have is:

• I have two packages, GPL and AGPL (and so two setup.py files, that the makefile moves in and out)
• I have some resources from docs I need to distribute that the makefile moves around

I always found the documentation for setuptools to be not super clear; so I admit there may be bits I missed...

[asottile]

hmmm I see -- make one the default? that seems like the easiest choice

you're probably best to leverage the utilities provided by setuptools rather than make -- for example package_data (here's a short tutorial about handling package data)

[florianschanda]

@asottile OK, thanks for the pointer. I will see what I can do.

I forgot to mention one additional constraint, in that github requires docs to be in docs/ and I definitely don't want to store duplicate copies of anything (hence I do some magic in the makefiles)...

[asottile]

what does github do with ./docs? is that for gh-pages? you can change the github pages prefix now (they added that feature a year or so ago)

[florianschanda]

Yes, for GH pages. But, I can only choose from precisely two options: a separate branch or the /docs directory or /. If there is an option to change it to something else, it's not obviously visible.

[asottile]

Yes, for GH pages. But, I can only choose from precisely two options: a separate branch or the /docs directory or /. If there is an option to change it to something else, it's not obviously visible.

ah I see, I've never actually clicked the dropdown but assumed it allowed any directory 🤦 my bad