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

pre-commit: Add more jobs #521

Closed
wants to merge 1 commit into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 9, 2024

% grep http .pre-commit-config.yaml

Learn more about this config here: https://pre-commit.com/

% git diff master --name-only

.pre-commit-config.yaml # I modified this file
pyproject.toml # This file was modified by pyproject-fmt and validated by validate-pyproject
requirements.txt # This file was modified by requirements-txt-fixer
test_app/models.py # This file was modified by auto-walrus
waffle/management/commands/waffle_delete.py # This file was modified by auto-walrus
waffle/models.py # This file was modified by auto-walrus
waffle/tests/test_templates.py # This file was modified by codespell as started in:

@ulgens ulgens self-requested a review November 9, 2024 22:10
Copy link
Member

@ulgens ulgens left a comment

Choose a reason for hiding this comment

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

Please try to open up the changes you made to the discussion. Ruff PR was merged almost immediately, which is sometimes a good thing, but I'm not sure about that case. The content of this PR is similar: There is a dump of configuration in the repo that needs a thorough explanation of their purpose and selection.

@ulgens
Copy link
Member

ulgens commented Nov 9, 2024

@cclauss Let me rephrase my review:

This PR adds

  • 27 new pre-commit hooks under pre-commit-hooks
  • 4 new pre-commit hooks repo

Ruff PR added

  • 28 enabled ruff rulesets, including the ones for Airflow, numpy, and pandas
  • 28 disabled ruff rulesets
  • 9 linter settings

Having pyupgrade, flake8, black, etc., in place is good, but adding 96 different linter settings without discussing them first is not the best way to go.

@cclauss cclauss requested a review from ulgens November 9, 2024 23:15
Copy link
Member

@ulgens ulgens left a comment

Choose a reason for hiding this comment

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

Just gave my review, not sure why it was requested again.

@dancergraham
Copy link
Contributor

dancergraham commented Nov 10, 2024

Hello here are my first thoughts on this one

Adding pre-commit checks for things which commonly fail in CI is definitely a good idea. Are there any common problems?

Indiscriminately adding lots of checks before people can commit anything is definitely a bad idea. If we accept these then people can reasonably assume they are expected.

I think pre commit hooks should usually be a curated subset of the CI checks - @cclauss is that the case here ?

We should avoid unnecessarily adding things which make it harder to start contributing or significantly slower to commit.

I have not specifically assessed this set of pre-commit hooks to know where they fall within that range. The PR message above doesn't help me to assess them. As it stands I'd say thank you very much but no thanks unless someone has time to fill the gaps in information.

Copy link
Contributor

@dancergraham dancergraham left a comment

Choose a reason for hiding this comment

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

From the diff I can see this is enforcing rules which are not included in CI. Pre commit checks should only be a strict subset of CI checks to avoid adding noise to other people's PRs.

@cclauss
Copy link
Contributor Author

cclauss commented Nov 10, 2024

I have proposed this set of pre-commit hooks. If you tell me which ones you object to, I can remove them.

@dancergraham
Copy link
Contributor

I have proposed this set of pre-commit hooks. If you tell me which ones you object to, I can remove them.

All which are not part of ci, to avoid adding noise to PRs

@cclauss cclauss closed this Nov 10, 2024
@cclauss cclauss deleted the more-pre-commit branch November 10, 2024 20:08
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