-
Notifications
You must be signed in to change notification settings - Fork 256
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
Add more pre-commit hooks #1211
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a sign that the hook is wrong. It shouldn't be marking this file as executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file shouldn't have a shebang then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very brave claim to make.
Although I would perhaps use #!hint/python
as the shebang instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what sense though?
https://github.com/pre-commit/pre-commit-hooks/blob/main/pre_commit_hooks/check_executables_have_shebangs.py
checks if files with a shebang are executable.
If that's not desired they shouldn't have one, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyhow, should be rebased and resolved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the sense that you're making the very brave and intimidating claim that just because the pre-commit-hooks project says something is "bad", that means it is bad.
Maybe the hook is wrong, like I originally said, and it's totally fine and desired for python files to have shebangs, whether they are marked executable or not. For example, text editors may use them as syntax highlighting hints. Okay, granted, that is more likely to actually matter for a python-formatted "rc" file than a .py file, but the general point remains.
On the other hand, I'm a pretty biased person -- I think that the python-pre-commit tool is a bad tool, badly designed, badly fitting into git, and encourages bad mishandling of software installation at a technical design level. I also think that it's a large collection of bad recommendations and incorrect ideas about what to check, so it's in bad taste to boot.
So I guess feel free to take everything I say with a grain of salt. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation - could you live with the current state though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Hi, could you rebase this on current master to verify that everything still passes? |
Hi @zmedico looks good to me. |
Also, add the fixes coming with them.