Skip to content

Remove the fail-fast option in pre-commit, fix existing issues#749

Merged
sbrunner merged 6 commits intomasterfrom
no-fast-fail-pre-commit
Apr 10, 2025
Merged

Remove the fail-fast option in pre-commit, fix existing issues#749
sbrunner merged 6 commits intomasterfrom
no-fast-fail-pre-commit

Conversation

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator

Apparently the fail-fast option permitted for some error to go unfixed.

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator Author

Added the pre-commit ci job so it runs in CI and check, fast fail was not the only culprit.

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator Author

The exit with error code 2 and no indication is very annoying, do you know how to fix this @sbrunner ?

@sbrunner sbrunner force-pushed the no-fast-fail-pre-commit branch from 00f74fc to 1bd83db Compare April 9, 2025 16:31
@sbrunner
Copy link
Copy Markdown
Member

sbrunner commented Apr 9, 2025

I found the test module which caused an issue, but I didn't succeed in finding the exact cause of the error, and I should stop for this evening!

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator Author

I think there's an underlying issue in requirement_detector because the directory structure is like this:

/home/pierre/prospector/tests/tools/pylint/pylint_configs/
├── multiple
│   ├── __init__.py
│   ├── pyproject.toml
│   ├── setup.cfg
│   └── testcode.py
├── pylintrc
│   ├── __init__.py
│   └── testcode.py
├── pylintrc2
│   ├── __init__.py
│   ├── pylintrc
│   └── testcode.py
└── pyproject
    ├── __init__.py
    ├── pyproject.toml
    └── testcode.py

And we're trying to reach /home/pierre/prospector/tests/tools/pylint/pylint_configs/setup.cfg (should probably be /home/pierre/prospector/tests/tools/pylint/pylint_configs/multiple/setup.cfg ?)

@sbrunner sbrunner force-pushed the no-fast-fail-pre-commit branch 2 times, most recently from 0d02a9d to 39eb520 Compare April 10, 2025 06:53
@sbrunner sbrunner force-pushed the no-fast-fail-pre-commit branch 4 times, most recently from 7a93a1c to e0b0e26 Compare April 10, 2025 08:24
@sbrunner
Copy link
Copy Markdown
Member

I get a pretty nice solution!

- types-PyYAML
args:
- --summary-only
- --zero-exit
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, why do we use this argument?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that the output of prospector messed with the output of pre-commit without it. Either causes formatting issues or drowns out other hook problems? I don't remember, maybe I didn't add it. I have no preference either way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will try to remove it in another pull request :-)

Copy link
Copy Markdown
Collaborator Author

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great ! I suggested an optimisation as we launcher the other hooks with pre-commit ci.

@sbrunner sbrunner force-pushed the no-fast-fail-pre-commit branch from e0b0e26 to 86c045e Compare April 10, 2025 11:44
@sbrunner sbrunner force-pushed the no-fast-fail-pre-commit branch from 86c045e to b7a0209 Compare April 10, 2025 11:51
@sbrunner sbrunner merged commit c326793 into master Apr 10, 2025
7 checks passed
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.

3 participants