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

Husky removal #245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Husky removal #245

wants to merge 1 commit into from

Conversation

pgiraud
Copy link
Member

@pgiraud pgiraud commented Dec 30, 2024

Fixes #244

@pgiraud
Copy link
Member Author

pgiraud commented Dec 30, 2024

For the documentation, my intention is to update powa-team/powa#181

@rjuju
Copy link
Member

rjuju commented Dec 30, 2024

Isn't that way over engineered?

I was expecting just a new pre-commit script either in a new script directory or anywhere else, that contains mostly the current husky pre-commit plus ruff command and a line in some readme saying that you can symlink it in the .git directory if you want to check most of the same stuff as the CI and that's done.

@pgiraud
Copy link
Member Author

pgiraud commented Dec 30, 2024

One important advantage of the pre-commit framework is that it runs only against the staged files. I don't think that a (bash) script that doesn't check all the files (including untracked ones) or unstaged changes will be simple to write. I'm not really eager to write it myself. As well as I don't want the pre-commit hook to check for errors in "not staged" changes or untracked files.

We can at least make the checks consistent in git hooks and CI by calling pre-commit run --all-files in the CI to make sure we check the same things.

@rjuju
Copy link
Member

rjuju commented Dec 30, 2024

I don't think that's a big problem. Storing random files in a git project is a very bad habit and can only lead to problems (adding wrong files, forgetting to add files and so on). Also, the script would be an opt in thing to be able to check stuff before the CI. Personally the first thing I would do would be to not enable it as it has only been annoying me since its introduction. I would just invoke it before pushing, or more realistically push, have a CI error, run it and push again. Annoying, but less that current behavior.

@pgiraud
Copy link
Member Author

pgiraud commented Dec 31, 2024

Don't you use git add -p to add some chunks to your commits?

I agree that husky is too aggressive because it installs itself automatically. In comparison, pre-commit is an opt-in. It's developer's choice to install hooks or not.

For now, my proposal is to first remove husky (PR updated). The CI is already configured. If there's a an error, you are still able to run the failing command locally.

@rjuju
Copy link
Member

rjuju commented Dec 31, 2024

Don't you use git add -p to add some chunks to your commits?

I do, and also review my own changes while at it before submission. But my experience reviewing other people PR is that most of people likely don't. That being said, relying on that is also a great way to forgot adding new files. You can of course install some kind of git prompt to help you make sure you don't forget to stage files, but then this is only helpful if you don't have random files laying in the repo, which brings back to my original "storing random files is a bad habit".

I agree that husky is too aggressive because it installs itself automatically. In comparison, pre-commit is an opt-in. It's developer's choice to install hooks or not.

It's also the developer's choice to install it as wanted. In my case while I don't like pre-commit hook as I tend to commit frequently in-progress work (including when I need to switch to another branch), but I would totally symlink such a script as a pre-push, since at that point I do want to make sure all the easy mistakes are fixed.

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.

Remove husky dependency?
2 participants