-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
remove unused black pre-commit hook and blacken docs #2737
Conversation
@graingert Hi. I'm not the maintainer of this repository, but I'll give you some advice. Your commits contain too many redundant codes. If you are using VScode, I think the python linter works automatically when you save the files. Please note the tests below that must be passed in order to be merged and the amount of code you have modified(+13,393 −9,314). |
Hello, first thanks for the advice! However in this case I was simply running "git commit" when these automated changes were applied by the configuration in this repository |
Oops.. I'm not sure if it's okay to include this change because I'm not the maintainer of this repository, |
@jaraco I have a question!
I'm incredibly surprised that 198 files have been modified in this PR, because of |
I'd also be happy to draft a new PR with my doc syntax error fixes, and the black configuration removed |
There are many things going on here. This project derives from jaraco/skeleton, so that’s where the black configuration comes from. It’s is meant to be disabled because this project hasn’t actually adopted black yet, though I plan to do so soon. I don’t experience the problems because I haven’t opted in to pre-commit for my local checkout of this repo. Is that an option for now (opt out of pre-commit for Setuptools) or do you have another tool that automatically installs precommit? |
03ebe12
to
a51b0b3
Compare
@jaraco I use existence of |
Totally fair and reasonable. I guess what I'm asking is that you disregard that call temporarily for this project. |
I think it's still worth applying blacken docs though |
I'm slightly opposed to running pre-commit in CI. It will add a new workflow and thus noise to the UI. For example, it will double the number of lines that appear under the actions list: And while that might seem like a minor annoyance, it's caused me real difficulty when I go to that page to check out the status of a push or tag. When I quickly glance at that page, I have to mentally filter out "pre-commit" lines. I've previously had to do this for a different workflow I'd enabled called "automerge", which I recently removed because it was not useful, and I was relieved that I no longer had to mentally filter out those unhelpful workflow results. That said, if the best thing to do for a project that has pre-commit hooks is to run them in a workflow, I may consider doing that, but I'd want to do it in jaraco/skeleton, so I'm not having to manage it for each project separately. I can see that I added the pre-commit hooks in jaraco/skeleton@d0f07a4#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9. Unfortunately, I didn't leave myself enough breadcrumbs to track what instigated me to add that. I think someone suggested in another project that it's something every project should have and so I adopted it experimentally. I'd like to ponder this some more, but I appreciate you sharing the suggestion. |
Ok I deleted the extra workflow. I still think it's worth doing the doc fixes here |
I agree that a lot of the changes herein are worthwhile. I'm a little less certain about others. In particular, there are two changes that blacken-docs makes with which I'm uncomfortable and likely to revert:
I would like to keep the other changes. I hate to be pedantic for what I imagine you thought was a straightforward and obvious improvement, so I'll accept responsibility to manually back out the unwanted changes. Thanks for the contrib! |
For that specific example you can use |
1ed1054
to
22cf1c5
Compare
It's probably worth finding an alternative to check the syntax of these docs |
22cf1c5
to
db3dd22
Compare
Summary of changes
pre-commit is configured in this repo. This means whenever I commit I automatically apply these fixes.
Because pre-commit is not run in CI the compliance has drifted, and so when I commit I get a huge number of changes!
Closes
Pull Request Checklist
changelog.d/
.(See documentation for details)