Adding style checkers to the cookiecutter

[JoaoRodrigues]

Hi all,

As suggested by @jchodera and @janash on Twitter, it would be nice to have some sort of linter added to the cookiecutter. There's already some configuration for flake8, but it requires the user to install & run it manually.

I've been using pre-commit for my projects and it's been a great experience. I was looking into adding it to this cookiecutter tempalte but I'm not sure how to proceed. We'd need to install pre-commit from either pip or conda (I guess that depends on what the user chose), but that assumes the user is in the right environment (for conda), which I am not sure is the best thing to assume.

If this is an interesting thing to add, how would you consider moving forward with this? Would just doing either pip install or conda install be an option?

[Lnaden]

I think based on the discussion on twitter, I think we are wanting to implement black instead of flake8. The flake8 in the setup.cfg came from one of us devs using some very common yet very helpful settings in our own config files which we wanted to have present in case someone decided to use flake8.

Does that sound like an accurate summary of what we talked about?

[dgasmith]

Well, we have YAPF at the moment. Switching to Black is likely a good idea as it is gaining more traction, but is still disruptive.

Perhaps adding a small discussion for a user to optionally run flake8 would be a good idea. We would need to turn off all of the formatting warnings (F401, E203, E501, E231, etc) however since neither YAPF, Black, (or really anyone) fully formats to PEP8 style.

Something like pre-commit could be neat, but would likely need to be an option since it can be a divisive for both new and old programmers and largely depends on personal flavor.

[janash]

@bennybp has pointed out to me that pre commit hooks are not pushed and pulled with a repository, so its seems that is not the best solution for the cookie cutter (I think we could add one when the cookie cutter runs initially as an option, but there would be no way to enforce this across different copies of the repository). I don't think we would ever want to add a pre commit hook automatically regardless, probably would only want it to be an option if it was included.

I do think adding something like this to the Python Coding Style section in the Best Practices workshop could be useful (and perhaps as documentation in the cookie cutter).

John Chodera mentioned this GH workflow which OpenFF has for running flake8 and black. (https://github.com/openforcefield/openff-evaluator/blob/87018e233ae1d0bc2db77655a9548b2d0891271c/.github/workflows/lint.yaml) This kind of check could be included in the cookiecuttter.

[mattwthompson]

+1 for more instructions on how to set up pre-commit, possibly among a list of linting services, but not for having it installed by default

[JoaoRodrigues]

Hi all,

Thank you for all the comments both here and on Twitter.

I believe there are two aspects of integrating style checkers: local development and online CI. Adding a GHA workflow to run flake8/black/yapf would be great and I'd be happy to contribute that. However, and this comes from personal experience, I think it's not productive not to have a local check - I hate having to wait for GHA or Travis to tell me if I missed an extra space somewhere. This is the reason I brought up pre-commit. As @janash said, Git hooks are not pushed/pulled (and that's great), so pre-commit is (right now) the nicest way to setup/share them.

Based on your comments and on the current setup of the cookiecutter, I believe having a prompt asking if the user wants flake8 or flake8+black, and another asking if he wants to setup whatever they chose locally using pre-commit would be the best course of action. For the latter we'd still need to figure out how to install pre-commit.

[dgasmith]

For new projects I use:

autoflake -ir --remove-all-unused-imports --ignore-init-module-imports --remove-unused-variables
isort
black
mypy

It's important to be in this order since isort/black sometimes disagree.

I still try to stay away from pre-commit generally as its doesn't allow the desired control, to make it simpler I build a Makefile. Its not new or flashy, but works on every POSIX system in existence and generally well known.

[janash]

I am returning to this thread because I was looking into precommit today. I didn't previously understand the difference between the pre-commit hook and pre-commit software.

I've been playing around with the precommit software today, and while I think it seems like a nice idea, I found it very hard to use and configure. The documentation is severely lacking when it comes to using conda. I was not able to use it with conda initially because I do not have a "system wide" install. I have seen that black itself uses precommits to run black on their commits, so it seems to be a strategy that is somewhat widely used.

I ran into a few other problems in addition to the "system-wide" conda problem. I was able to make it work in an extremely hacky way, but had no luck in finding guidance in the pre-commit documentation.

@JoaoRodrigues - do you have any guidelines on how to better use with conda? This seems very convenient to use with commits, but seems too hard to include with the cookiecutter at this point (unless I'm missing something)

FYI I was using it just to run black.

[janash]

Replying to my own message - I got it working today. I actually just didn't need the language_version keyword suggested by the black repo (https://github.com/psf/black#version-control-integration). It's actually very simple once that keyword is removed...

I would be in favor of adding this as an option to the cookiecutter. Note that just because there is a .pre-commit-config.yaml in the repo, people do not have to install or use pre-commit if they don't want to. In order to use pre-commit, you have to both install pre-commit, and run pre-commit install in the repo, so there isn't a chance of someone installing it accidentally. I also don't see too much harm if someone doesn't install pre-commit and use the hooks - then it is essentially like the pre-commit yaml isn't there (or the way things are now).

As far as what to run with precommit - I've only tried black, but I think a combo of black + flake8 would be good.

[JoaoRodrigues]

Hi @janash,

Thanks for looking into this. I agree that adding a pre-commit YAML config file to the cookiecutter template repo is a good idea. Then you can simply tell the user that to use it, they need to run pip & pre-commit install. That way it remains entirely optional.

Here's a "live" example of one of the setups I use, from Biopython, that has quite a few things (black, flake8 with plugins, docstring checkers, etc). It might be overkill here but you do put an emphasis or writing good docstrings during the course so maybe not!

[mattwthompson]

There is now a service that automagically runs pre-commit in CI and applies fixes: https://pre-commit.ci/ It's "zero configuration" in the sense that the CI service uses the YAML file that already exists in a repo - project maintainers just need to install the app. And it works pretty much the same if a project has just a few hooks (i.e. if the cookiecutter ships with just two) or a bunch of them.

In the case of developers that are already using pre-commit as intended, it adds nothing but a ✅ that takes around ~30 seconds to appear. For those that aren't using pre-commit and commit something that doesn't quite pass the checks, a bot automatically applies those fixes (i.e. will run black on the files) just as if the developer was using pre-commit themselves.

I think this helps with some issues people have had adopting pre-commit in the past, i.e. keeping a project's code style in sync across a project without requiring all developers install another tool (though in my experience it's lightweight and quick, I get why some people would prefer not to). Since a bot applies the fixes, it doesn't require anybody check out code just to run an auto-formatter or coordinate code quality fixes during reviews - by the time a PR hits a review stage, everything should be fixed and in sync. And since it's automated, it doesn't require newcomers to actually understand what's going on under the hood; it's probably confusing to people just learning how git works to also understand how hooks fit into a development workflow - or at least I know it was for me. This should bypass that issue altogether.