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

🩹 Fix pre commit hook manifest #287

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

s-weigand
Copy link
Contributor

@s-weigand s-weigand commented Oct 5, 2024

pre-commit just released v4.0.0 with the breaking change that language: python_venv was removed.
This breaks docformatter hook config at parse time, even for users that don't use docformatter-venv (e.g. I use the "normal" docformatter hook).
The error is:

An error has occurred: InvalidManifestError: 
==> File /home/runner/.cache/pre-commit/repocernquz3/.pre-commit-hooks.yaml
==> At Hook(id='docformatter-venv')
==> At key: language
=====> Expected one of conda, coursier, dart, docker, docker_image, dotnet, fail, golang, haskell, lua, node, perl, pygrep, python, r, ruby, rust, script, swift, system but got: 'python_venv'
Check the log at /home/runner/.cache/pre-commit/pre-commit.log

In addition to removing docformatter-venv I also added the pre-commit built-in validate_manifest hook to check the hook manifest.
And for the linter CI to pass I had to add some additional fixes, since it seems to be broken since Aug 9, 2023 (most likely due to a tool update but the CI logs are already gone)

Additional info:

language: python_venv has been deprecated Feb 4, 2023 and should have emitted a warning since then based on the code.
However, in my last CI run using that passed pre-commit successfully and without warning (pre-commit==3.8.0).

...
[INFO] Installing environment for https://github.com/PyCQA/docformatter.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/MarcoGorelli/absolufy-imports.
...

Furthermore the built-in validate_manifest hook in pre-commit==3.8.0 also didn't have an issue with language: python_venv.
So it seems like there was no easy way to get a heads-up, except closely following the pre-commitchangelog.

Edit:
Now I get why there was no warning when pre-commit did setup the env for docformatter, it's because I used the docformatter hook and the warning would only be shown when someone actually uses the docformatter-venv hook.

@s-weigand
Copy link
Contributor Author

Also thanks a lot for the project and its maintenance ❤

@s-weigand
Copy link
Contributor Author

See also #289

@dakl
Copy link

dakl commented Oct 7, 2024

This would be great to get in! 🙌

@webknjaz
Copy link

webknjaz commented Oct 7, 2024

@weibullguy you merged a topic-related PR in the past (#266). Perhaps you could accept this one too?

lebrice added a commit to mila-iqia/ResearchTemplate that referenced this pull request Oct 7, 2024
- PyCQA/docformatter#289
- PyCQA/docformatter#287

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
lebrice added a commit to mila-iqia/ResearchTemplate that referenced this pull request Oct 7, 2024
@grochmal
Copy link

grochmal commented Oct 7, 2024

Great work @s-weigand , just hit this today. I search for it and there is already a PR in the works. Thank you!

lebrice added a commit to mila-iqia/ResearchTemplate that referenced this pull request Oct 7, 2024
* Begin improvement of refs generation

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Proof of concept works! Converts backtics to refs

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Simplify, trim stuff

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Add a test for the autoref plugin

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Remove unused code paths

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Update pre-commit config?

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Try a fix for broken docformatter pre-commit hook

- PyCQA/docformatter#289
- PyCQA/docformatter#287

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Restrict pre-commit version for now

Trying the same approach as DSD-DBS/capella-dockerimages@6e38a0c

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Temporarily don't use existing regression files

There's this issue at the moment with some reproducibility test
that cause the CI to fail.

I'll make another PR to address this, but for now, I'm turning off
the 'shared regression files in $SCRATCH' feature.

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Add test to reach 100% patch coverage

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

---------

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@danilogco
Copy link

I hope it gets approved soon. Gg

@Woody1193
Copy link

This is currently breaking all our workflows so I do hope it's approved soon.

@snarayan21
Copy link

Breaking our workflows as well, hope this gets merged + released soon!

@webknjaz
Copy link

webknjaz commented Oct 8, 2024

Folks, please be mindful that every new comment sends a notifications storm to many people. Especially, since one maintainer has been pinged already. Those who commented before and those who just subscribed to updates. A bunch of “me too” / “+1” / “when will this get merged” / “when will this get released” duplicate notifications is definitely not something that is useful for every single person to receive, nor does it help get this merged. It just leads to having a lot of pointless notifications in people's inboxes, which is unhelpful to say the least. Please avoid adding more similar comments that are not contributing to advancing the topic of the PR. Please also be mindful that the maintainers might not be able to do anything for a while, regardless of the number of comments. Thank you in advance.

@s-weigand
Copy link
Contributor Author

For the impatient, there are 3 workarounds until maintainers have time:

  1. Set an upper bound of pre-commit<4
  2. Comment out the hook in your .pre-commit-config.yaml
  3. Use the fix commit from my PR branch (you should not use branches from random ppl off the internet 😅)
    E.g.:
  # TODO: Switch back to upstream docformatter 
  # after https://github.com/PyCQA/docformatter/issues/289 is fixed
  - repo: https://github.com/s-weigand/docformatter
    rev: 1ec30b7
    hooks:
      - id: docformatter
        additional_dependencies: [tomli]
        args: [--in-place, --config, ./pyproject.toml]

Also, be patient with maintainers, this is FOSS and no one is entitled to anything. 😉

@webknjaz
Copy link

webknjaz commented Oct 9, 2024

Alternatively, you can drop the dependency on GitHub and replace it with the last PyPI release:

---

ci:
  autoupdate_schedule: quarterly  # low frequency to reduce maintenance noise

hooks:
- repo: local
  hooks:
  - id: docformatter
    name: docformatter
    description: Formats docstrings to follow PEP 257.
    entry: python -Im docformatter
    additional_dependencies:
    - docformatter == 1.7.5
    args:
    - --in-place
    language: python
    types:
    - python

...

webknjaz added a commit to ansible/awx_plugins.interfaces that referenced this pull request Oct 9, 2024
This is necessary since it uses outdated config format for pre-commit.

Refs:
* PyCQA/docformatter#287
* PyCQA/docformatter#289
@weibullguy weibullguy merged commit 06907d0 into PyCQA:master Oct 9, 2024
7 checks passed
@s-weigand s-weigand deleted the fix-pre-commit-hook branch October 9, 2024 19:24
mmcdermott added a commit to mmcdermott/nested_ragged_tensors that referenced this pull request Oct 10, 2024
thedoubl3j pushed a commit to thedoubl3j/awx-plugins that referenced this pull request Oct 10, 2024
This is necessary since it uses outdated config format for pre-commit.

Refs:
* PyCQA/docformatter#287
* PyCQA/docformatter#289
mmcdermott added a commit to Oufattole/meds-torch that referenced this pull request Oct 10, 2024
lebrice added a commit to mila-iqia/milatools that referenced this pull request Oct 11, 2024
- PyCQA/docformatter#289
- PyCQA/docformatter#287

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
mmcdermott added a commit to mmcdermott/MEDS_transforms that referenced this pull request Oct 12, 2024
orviz added a commit to IFCA-Advanced-Computing/FAIR_eva that referenced this pull request Oct 16, 2024
orviz added a commit to EOSC-synergy/FAIR_eva that referenced this pull request Oct 16, 2024
lebrice added a commit to mila-iqia/milatools that referenced this pull request Oct 17, 2024
* Add test to reproduce issue

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Bugfix for mila code in WSL

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Fix issue in pre-commit hook

- PyCQA/docformatter#289
- PyCQA/docformatter#287

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Remove unnecessary mock for RemoteV2 in test

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Remove unnecessary xfail mark on Windows

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Adjust the regression test files following changes

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

* Add xfail for flaky integration test

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

---------

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
nikhilbadyal added a commit to nikhilbadyal/ghaction-telegram-uploader that referenced this pull request Oct 22, 2024
Removed doc-formatter until we have release for PyCQA/docformatter#287
azmeuk pushed a commit to yaal-coop/canaille that referenced this pull request Oct 25, 2024
xingzhongyu added a commit to OmicsML/dance that referenced this pull request Nov 15, 2024
@setu4993
Copy link

Bump. Been over a month since this was merged, would be good to create a new release with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants