Require black and isort for contributions #3329
Conversation
* Add explanation to CONTRIBUTNG.md * Add sample pre-commit script * Check for correctly formatted files in CI
srittau
added
the
project: infrastructure
|
Should we land this soon? Currently not that many PRs are open, and I'd love to get this in so I don't have to worry about how to format stubs. cc @CraftSpider and @hauntsaninja |
|
Assuming we're doing this*, now seems like a good time to me too! I know it's not the Black way, so hiding my small dissent. Despite liking it in code, I've personally come to dislike one line per arg in stubs. In particular, I find it much harder to compare long overloads (e.g. |
|
@hauntsaninja Quick reply to your gripes: I am not totally on board with all formatting decisions of black (or PEP 8), either, but in the end it's more important to me to have consistent formatting than one that perfectly aligns with my tastes. |
|
It seems like we're not likely to get much lower, I'm down to land this soon. As for the formatting style, I also find one line per arg int stubs less readable, but consistency is generally more important. If most others also find it inconvenient, that makes it definitely a known papercut, but if that's the only one I think that's an acceptable trade-off for the consistency gained.
|
|
We can land this once #4224 lands. |
The previous version automatically reformatted the changed files. This can be problematic when only committing partial files. The new version is safer only checks the changed files, with the option to change the files in the comments.
|
We applied Black/isort in #4287. I'm going to fix up this PR and merge it after lunch (unless @hauntsaninja or @srittau gets to it first). |
should have re-run black after moving the type ignore
|
Hm, isort is failing in CI, maybe it's not picking up our config from pyproject.toml? |
|
Ah |
Rather than installing |
|
@Eric-Arellano you're right, I was too lazy to figure out how to combine the |
|
Looks like isort is still failing |
|
OK, now there's another isort error, which is apparently because isort behaves differently depending on whether or not For now I'm going to remove isort from CI and land the Black part of his PR. We can try to figure out a less fragile isort config in a separate PR. |
|
CI passed, I'm landing this now. We can discuss how to turn on isort in #4288. |
This is a draft, which we should merge only after the following are solved: