yellowbrick icon indicating copy to clipboard operation
yellowbrick copied to clipboard

Implement black code formatting as pre-commit

Open bbengfort opened this issue 7 years ago • 7 comments

In order to avoid style-related comments when discussing code with contributors, I propose that we implement the black python code formatter as a pre-commit requirement for development.

This will potentially have a large impact on our current code-base, so should be undertaken right before a release to ensure that we don't end up with large merge conflicts. Also for discussion:

  • what styles do we want to customize with black?
  • is black in its beta stage stable enough to use with our project?
  • do we set this up as a pre-commit in GitHub/Travis/locally?

bbengfort avatar May 29 '18 18:05 bbengfort

@ambv -- if you have any suggestions how we should go about this for our project or know of any open source libraries that have implemented black, we'd appreciate your thoughts!

bbengfort avatar May 29 '18 18:05 bbengfort

There's many projects, last ones I learned about were:

  • http://github.com/pypa/warehouse/
  • https://github.com/pytest-dev/pytest
  • https://github.com/fabric/fabric
  • https://github.com/hynek/prometheus_async

General guidelines:

  1. Make one commit with only the automatic formatting. Afterwards you'll be able to skip over it easily with git hyper-blame or git blame $BLACK_REV^ -- $FILE.

  2. Avoid leaving open pull requests. If you do, after landing the blackening commit, blacken all pull requests, too. They shouldn't conflict then.

  3. Set up enforcement with pre-commit or CI (you can run black --check on Travis or similar).

  4. Don't forget the repo badge ;-)

ambv avatar May 29 '18 18:05 ambv

Awesome, thanks @ambv!

bbengfort avatar May 29 '18 18:05 bbengfort

Note: Moving this from v1.0 to v1.1 — we've run black on all the files that are about to be merged in #957; this issue concerns implementing a pre-commit hook that automatically runs black ahead of git push

rebeccabilbro avatar Aug 28 '19 23:08 rebeccabilbro

It's been a while since this issue was updated. Is this still something you want to include? I'm happy to put together a pre-commit hook in a PR, if it's still of interest.

stefmolin avatar Jul 27 '22 01:07 stefmolin

@stefmolin that would be great. I guess it's been quite a while since we've thought about this. The precommit hook would require users to have black installed before they could commit right? Or would you only run black if it was available? It seems like we'd also need to update the contributor's guide documentation and add black to one of the requirements.txt files - possibly the docs/requirements.txt?

bbengfort avatar Jul 29 '22 18:07 bbengfort

@bbengfort - I'll be implementing this using pre-commit, which makes the setup and configuration of pre-commit hooks easy :smile: So, to answer your first two questions: a user won't need to install black at all actually, they will just need to install pre-commit, which will pull down black (and any other checks you want to run) for them.

As for the requirements.txt files, that is going to be a question of how strict you want to be. I'm a first-time contributor, so I don't know what kind of checks you have automated on PRs, but if you don't have black in there, you might want to require people to set this up on their end. Aside from installing pre-commit, people will have to run pre-commit install to install the hook the first time, so the process wouldn't be automated even if you include it in the requirements.txt file.

I'll open up a PR with the config file and the instructions for installing the hook, and once you've had a chance to test it out, we can decide how to proceed on the requirements.txt changes and updating the contributor's guide documentation.

stefmolin avatar Jul 29 '22 23:07 stefmolin