ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
20.42k stars 1.5k forks source link

Create pre-commit hook for running black #589

Closed eulertour closed 3 years ago

eulertour commented 3 years ago

This will hopefully cut down drastically on the number of extra commits that are made just to run black. Instructions are here.

kolibril13 commented 3 years ago

Nice idea. For myself, I found for PyCharm a way to run black with a shortcut (see instruction) on the currently active window, which would give a little more control of what happens then when this is completely automated.

MysaaJava commented 3 years ago

I'll try to do something about this. I need to make some hooks for the i18n thing, so i take that as a test ^^.

MysaaJava commented 3 years ago

Hello. Here's a quick update on what i've found. First, there is currently no way of installing the hook directly,, for security reasons (you don't want to clone a random repo and have your computer wrecked up by some troll code). So the developper will have to install the script manually in the .git/hooks folder. I'll make a little paragraph in Contribution to help weth that. But where should the file be stored. I think of three locations:

Also, there is two types of hooks that can fix the use: pre-commits hooks that run every time you make a commit (and runs black, which lengthen the commit time) and pre-push hooks that runs black before pushing. If you think both are useful, we can still upload both and let the devs choose the one they prefer.

What is left to do: setup a script that detects the python environnement and uses the right black version. This script will be re-used in the docs makefiles, because there is currently issues of the wrong manim version being used.

Thanks for reading !

naveen521kk commented 3 years ago

If the aim is only running black, then we have https://pre-commit.com which can be used. We would be adding a file for the version. The Dev's will run pre-commit install after cloning so that it is configured. I personally hate it though.

MysaaJava commented 3 years ago

I'm not a huge fan of adding a depedency just to run black. And this package is only a wrapper for git hooks. I've looked at it, and foundthat et did not solve the environnement issue.

naveen521kk commented 3 years ago

Instructions here suggests that. Also, adding it as a dev-dependency doesn't hurt though, maybe be helpful for some but not all.

MysaaJava commented 3 years ago

The pre-commit seems to work. But one problem is that, when black changes a file, the return code is set to nonzero and the commit action fails, even though the files are now correct. And because of that, i don't know if precommit will work when IDEs runs git, they may send a confusing warning. I can write a pre-commit script that will not face this problem, but then, devs will have to install it manually. Plus pre-commit gets the python executable location based on when was pre-commit install launched, so one cannot change between evironnements (multiple poetry envs, or poetry/system/ide). You may have seen that i'm not really partial to the pre-commit solution, but if most devs think it is the better option, i'll write a paragraph in docs' contribution page. What i propose is that i make a pre-commit script (one for each os) that the dev can configure, maybe with a command-line script. That script will find black in configured locations, and will skip black if it is not found. I also propose adding a script that checks if pytest has been run and messages the user if it has not. This script will be hosted on github or on any secured file hosting service, and the user will only have to run curl URL | source or something like that. What do you think ? Also (last question, i promise), i can either make pre-commit hooks or pre-push hooks. What is the best do you think (i can still do both)