mbarkhau / pylint-ignore

MIT License
14 stars 8 forks source link

Integration with pre-commit (add a `.pre-commit-hooks.yaml` file) #9

Closed jamesquilty closed 2 years ago

jamesquilty commented 3 years ago

Thanks for writing pytlint-ignore! I'm trying it out now and I'd like to integrate it with pre-commit https://pre-commit.com/ both locally and in the GitLab CI pipeline. This will require me to write a repository local hook because the "official repository for the linter doesn't have the pre-commit metadata". This isn't necessarily a big problem, but it's pretty simple for that metadata to be added to this repo as a .pre-commit-hooks.yaml file. Would you be open to adding that metadata file, please?

mbarkhau commented 3 years ago

Sure, I guess it would be pretty much like this?

https://github.com/PyCQA/pylint/blob/main/.pre-commit-hooks.yaml

jamesquilty commented 3 years ago

@mbarkhau yes, that should work... the only uncertainty is around the passing of the configuration file. Here's a local hook that works, written by a student who is working with me:

  - repo: local
    hooks:
      - id: pylint-ignore
        name: pylint-ignore
        entry: pylint-ignore --rcfile=.pylint-ignorerc  # --update-ignorefile
        language: system
        types: [python]
        require_serial: true

As you can see, we were explicitly coding the rcfile into entry: because we believe that pylint-ignore doesn't search for a default config file but has to be passed one explicitly as an argument. Also, the only way to generate pylint-ignore.md for all Python files in a directory hierarchy appeared to be to execute pre-commit run --all-files with the --update-ignorefile flag uncommented in the above entry in .pre-commit-config.yaml. Executing pylint-ignore -rcfile=.pylint-ignorerc . doesn't work unless there's an __init__.py file present, behaviour originating from pylint.

On the positive side, this entry in .pre-commit-config.yaml functioned correctly locally and in a Docker container on the CI pipeline.

I think the outstanding question is how pylint-ignore sources an rcfile by default?

mbarkhau commented 3 years ago

If I remember correctly, all arguments to pylint-ignore (except for its own arguments --update-ignorefile and --ignorefile) are simply passed through to pylint and should behave exactly as documented for the pylint command

jamesquilty commented 3 years ago

Thanks, that simplifies matters! The How it Works section gave the impression that a config file (setup.cfg is given as a "minimal config") had to be passed. So, the local hook above really only needs entry: pylint-ignore. I think that a .pre-commit-hooks.yaml file

- id: pylint-ignore
  name: pylint-ignore
  entry: pylint-ignore
  language: python
  types: [python]
  require_serial: true

would work. If you can wait until next week, I should then have some time to try it with pre-commit try-repo on a clone of this repository.

jamesquilty commented 2 years ago

A little late in the week, but I just tried the proposed .pre-commit-hooks.yaml above in a clone and it worked a treat:

$ pre-commit try-repo ~/pylint-ignore --all-files
[INFO] Initializing environment for pylint-ignore.
===============================================================================
Using config:
===============================================================================
repos:
-   repo: pylint-ignore
    rev: 1bba22c0feea07b9113d176dc0b2dbce8b3b44f4
    hooks:
    -   id: pylint-ignore
===============================================================================
[INFO] Installing environment for pylint-ignore.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
pylint-ignore............................................................Passed

The only change that I might suggest is amending the name: entry to

  name: pylint

so that the hook shows it's running pylint (which it is). That's somewhat a cosmetic suggestion.

Otherwise, I'd consider this all good to go. It's so small it hardly seems worth a PR, but I'd be happy to raise one if you prefer?

mbarkhau commented 2 years ago

I've spent half an hour now trying to learn how this works and I'm aborting now. Please prepare a PR which includes documentation.