The Wayback Machine - https://web.archive.org/web/20220918054041/https://github.com/scikit-learn/scikit-learn/pull/23822
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

MNT Updated pre commit hooks #23822

Merged
merged 2 commits into from Jul 6, 2022
Merged

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Jul 2, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Hi,
While setting up my dev environment, I realised that some pre-commit hooks in the pre-commit-config.yaml are outdated:

Any other comments?

Please let me know if the versions of these were 'outdated' for a reason. In this case, apologies for the PR! I checked previous issues, PRs and docs but I didn't see mentioned anywhere that the versions should stay fixed.

Thanks :)

@thomasjpfan thomasjpfan changed the title [MRG] Updated pre commit hooks MNT Updated pre commit hooks Jul 5, 2022
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Thank you for the PR!

Please let me know if the versions of these were 'outdated' for a reason.

Versions are pinned for stability. We have many PRs and having contributors updated their .pre-commit-config.yaml would be another thing to do. We can have a bot to auto update for contributors on PRs, such as https://pre-commit.ci/ but that introduces another layer of complexity.

That being said, the updates in this PR should not change the behavior.

XREF: changelog for flake8

@thomasjpfan thomasjpfan added No Changelog Needed Quick Review For PRs that are quick to review labels Jul 5, 2022
@EdAbati
Copy link
Contributor Author

EdAbati commented Jul 5, 2022

Hi @thomasjpfan, thank you very much for the review! :)

I was looking at the point 4 of the Contributing Guide - How to contribute:

Install the development dependencies:
pip install pytest pytest-cov flake8 mypy numpydoc black==22.3.0

Here black is pinned (and I have not updated it in the .pre-commit-config.yaml), but flake8 is not.
I thought that flake8 in the pre-commit may potentially return different results compared to git diff upstream/main -u -- "*.py" | flake8 --diff, in case the flake8 versions are not matching.
But you are right, it seems that the behaviour has not changed.

@thomasjpfan
Copy link
Member

thomasjpfan commented Jul 5, 2022

I expect flake8 to be a bit more stable compared to black. We use black with preview = true:

preview = true

which is more prone to formatting changes. Formatting changes from a black version update can lead to more work for contributors to sync their PR with main. We pin black's version to avoid this issue.

@lesteve
Copy link
Member

lesteve commented Jul 6, 2022

Thanks a lot, merging!

@lesteve lesteve merged commit 5d7079e into scikit-learn:main Jul 6, 2022
36 checks passed
@EdAbati EdAbati deleted the update-pre-commit branch Jul 6, 2022
Harsh14901 pushed a commit to Harsh14901/scikit-learn that referenced this issue Jul 6, 2022
ogrisel pushed a commit to ogrisel/scikit-learn that referenced this issue Jul 11, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this issue Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants