Page MenuHomeElementl

style: adopt pre-commit for Python black formatting
Needs RevisionPublic

Authored by rexledesma on Jul 2 2021, 7:38 PM.

Details

Summary

A contributor recommended that we adopt (pre-commit) for the formatting
steps that we usually test for in buildkite. We'll be migrating to Github
soon anyways, so adopting this toolchain will be great to replace what
arclint usually did for us.

Overall, pretty nice experience. pip install pre-commit and you're off to the
races.

I decided to format all python files rather than whatever we configured in the Makefile
before. Docs are formatted with line-length=78 for ease of reading.

Since this tool installs the hooks on its own, we remove dependencies of black from setup.

We'll use this tool to adopt the other linters we have as well.

Test Plan

buildkite, pre-commit run black --all-files

Diff Detail

Repository
R1 dagster
Branch
arcpatch-D8684
Lint
Lint Skipped
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 2 2021, 8:30 PM
Harbormaster failed remote builds in B33112: Diff 40813!

remove black step from grpc compile

use pre-commit in grpc compile step

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 6 2021, 4:45 PM
Harbormaster failed remote builds in B33235: Diff 40971!

I don't personally love git-hooks and find them to often be more of a hinderance than a help: https://www.thoughtworks.com/en-us/insights/blog/pre-commit-don-t-git-hooked

But if we do want to go in this direction, I think we should continue to make our use of black explicit in our setup.py (or at least in our make dev_install) so that developers can continue to run it ad hoc or automatically in their editors.

python_modules/dagster/setup.py
92

If black isn't installed, will editor linters/fixers generally work? I'm not sure how the common VSCode ones generally work, but I use ALE and it conditionally runs black based on whether or not it's installed. I rely on pyenv local so it'll automatically activate the appropriate virtualenv when I enter a directory - that way I don't automatically run black when working on projects that don't use black.

Take it with a grain of salt, but it looks like we backed away from pre-commit hooks once before as well:

https://dagster.phacility.com/D21

And later removed them entirely:

https://dagster.phacility.com/D558

Although I suspect the issue that caused us to remove them is no longer a problem.

I faintly remember some weird shit around statefulness of how pre-commit worked when we had it set up but im struggling to remember the details

make black has reference in contributing.mdx that'll need update

Overall, pretty nice experience. pip install pre-commit and you're off to the races.

you need to run pre-commit install too to get the hooks in to git installed

Should maybe post in #core-team or something to solicit feedback from others who may care or have past experience with pre-commit

.buildkite/dagster-buildkite/dagster_buildkite/steps/dagster.py
578

[1]

583

does pre-commit run black --all-files return a failing exit code if it generates changes? if not may need to go with [1]

.pre-commit-config.yaml
1–3

when exactly does this fire? on amends too?

I don't personally love pre-commit hooks but if we do this we should think about also adding mypy, isort

Well given that installing the pre-commit hooks is a choice, I think its safe to add the config file that meets our projects standards to ease that process for those that want it.

To make this change more agreeable & incremental - lets add the config file, but preserve the existing source of truth, CI, and dev setup portions. We can change the CI source of truth in the future if momentum for pre-commit is strong.

I don't have strong feelings about the files currently missed getting formatted, your discretion on how to handle those.

This revision now requires changes to proceed.Jul 23 2021, 3:25 PM