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

Issues with pre-commit hooks on Windows #305

Open
jrast opened this issue Nov 15, 2024 · 7 comments
Open

Issues with pre-commit hooks on Windows #305

jrast opened this issue Nov 15, 2024 · 7 comments

Comments

@jrast
Copy link
Contributor

jrast commented Nov 15, 2024

The pre-commit hooks added in 283eb40 rely on make being available, which is mostly not the case on windows.
However, the makefile simply calls ruff which can be done in pre-commit directly: https://github.com/astral-sh/ruff-pre-commit

Also I don't recommed to run the tests on each commit:

  • They take far to long for a commit (hang on windows)
  • Prevent to work on a PR which still has some issues.
  • Maybe there is a pre-commit option to only run the tests on the main branch?

Tests should be run manually and in CI to prevent merges with broken tests.

Typechecking seems to run fast but falls into a simmilar category as tests.

@jrast
Copy link
Contributor Author

jrast commented Nov 15, 2024

However, it would make sense to run ruff check --select I --fix (sort imports, see https://docs.astral.sh/ruff/formatter/#sorting-imports) and ruff format to automatically apply a coding guidline and prevent whitespace or reordering changes in commits.

@rickwierenga
Copy link
Member

fixed with 26664f9

using another external repo just to run ruff in pre-commit seemed excessive, so i just use the cli, hope that works

@jrast
Copy link
Contributor Author

jrast commented Nov 18, 2024

When using pre-commit, using the external repos is recommeded as the version of the hook is fixed. The hook environment is automatically shared between different projects (if the same version is used), making the overhead negible. Also the tool is only run on the files which where actually affected by the commit.

@jrast
Copy link
Contributor Author

jrast commented Nov 18, 2024

But other than this, the pre-commit hooks now run under windows. 👍

@jrast
Copy link
Contributor Author

jrast commented Nov 18, 2024

BTW: I still think that typechecking should not be done during pre-commit. It makes development cumbersome as the typing needs to be correct for every commit. For the moment I removed type checking from the config.

@rickwierenga
Copy link
Member

opened a new branch for you to evaluate changes before merging #310

@rickwierenga
Copy link
Member

commits were getting piled up on main so let's just get it as good as we can in 310 and then merge

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

No branches or pull requests

2 participants