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

Use require_serial: true as the default for our pre-commit hook, not -j1 #95

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

AlexWaygood
Copy link
Collaborator

Currently we have [--jobs=1] as the default args for our pre-commit hook. This is problematic, however, as repos that override args in their .pre-commit-config.yaml files (such as CPython) have to re-specify -j1 in their args: the value they provide for args overrides the default we give here rather than extending it.

This PR proposes that we don't give a default value for args, and instead give a default value for the require_serial key. Testing locally, using require_serial: true does seem to be slightly slower than using --jobs=1 for me. But the difference is barely perceptible, and I think this is a big usability improvement: it means we don't have to document that, if you're overriding args in your .pre-commit.config.yaml file, you'll need to make sure to add --jobs=1 in your override.

@AlexWaygood AlexWaygood merged commit ae69698 into main Oct 16, 2023
33 checks passed
@AlexWaygood AlexWaygood deleted the fix-pre-commit-again branch October 16, 2023 14:59
@AlexWaygood
Copy link
Collaborator Author

I'll do a patch release with this in it

@AlexWaygood
Copy link
Collaborator Author

https://pypi.org/project/sphinx-lint/0.8.1/

@AlexWaygood
Copy link
Collaborator Author

AlexWaygood commented Oct 17, 2023

Looks like this approach is also what black does for its pre-commit hook (black being another Python tool that uses multiprocessing internally): https://github.com/psf/black-pre-commit-mirror/blob/main/.pre-commit-hooks.yaml

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.

2 participants