[Twisted-Python] Black enabled in trunk
Hi all, Craig and I have been working to land a switch to The *Black* Coding Style <https://black.readthedocs.io/en/stable/the_black_code_style.html>. It's in! The tree has been reformatted <https://github.com/twisted/twisted/commit/bc96c774be6e307e8e3e4d39780d37b045...>. Some notable changes: * 3-2-1 blank lines are replaced by PEP 8 style 2-1-1. * String syntax uses "double quotes". Run this command to exclude the formatting from git blame <https://github.com/twisted/twisted/blob/trunk/.git-blame-ignore-revs>: git config blame.ignoreRevsFile .git-blame-ignore-revs There are two new Tox testenvs. To check style: tox -e black To adjust the formatting: tox -e black-reformat The formatting is checked by a new GitHub Actions lint built. There is some follow-up work planned: * Many PRs have conflicts (more on this below). * Various wiki pages need updates. I've done a few, but there are probably more. * The coding standard should be updated <https://twistedmatrix.com/trac/ticket/9957#ticket>. * The PR template needs an update <https://twistedmatrix.com/trac/ticket/9956#ticket>. * A few examples need reformatting <https://tm.tl/#9952> (they weren't formatted because they fail to lint). Inevitably, changes like this cause conflicts. For small PRs it's easiest to merge forward and then run tox -e black-reformat. For larger ones it can help to apply formatting before merge. To do this: * Run black on the files your branch changes (be sure to use Black 20.8b1, not an older version) * Commit the result, like `git commit -am "Fade to black"` * Add that commit to .git-blame-ignore-revs to avoid polluting git blame. * Merge forward. You can ignore formatting while performing the merge. * Then run `tox -e black-reformat` to ensure you are in sync. Commit any changes it generates. A big thanks to Hawkowl for kicking this all off and Craig for quick review turnaround. May we all be mildly displeased with the formatting, yet never have to review it again! ---Tom
Happy to hear we're making progress on this! I'm the tiniest bit grumpy that this went forward without having the follow-up work already landed or ready-to-land simultaneously — my experience suggests that the fact that this is now complete means that neither you nor Craig will ever contribute to Twisted, or in fact ever answer your email, ever again — but if we can get it all landed promptly I will be over the moon :-).
Could we possibly use something like this: https://github.com/cclauss/autoblack <https://github.com/cclauss/autoblack> to just do the formatting rather than "check" if it's correct? PR templates are all well and good but the best checklist item is the one that's already checked off...
Can we please get this bulleted list in the form of a tool that people can run against a PR, so we don't need everyone to write their own bespoke shell script for fixing up their Twisted PRs? This is the thing I'm the most worried about - I'd like to have it done automatically because I'm seriously worried about work in progress getting abandoned because somebody backs themselves into a corner with an apparently intractable mega-conflict by not knowing how to properly use Git to apply this sort of modification. -g
On 2020-09-14 13:43, Glyph wrote:
It might be worthwhile but I think there is some downside to CI frequently injecting commits (or amending them).
Is .git-blame-ignore-revs going to be the next 'newsfile' what with every PR adding lines to it? Also, for the record, this doesn't address GitHub blame and does require everyone set the previously mentioned git config since it doesn't go with the repo. :[ Though that's mostly an annoyance with git. Oh yeah... thanks! Cheers, -kyle
On Tue, Sep 15, 2020, at 7:26 AM, Kyle Altendorf wrote:
I'm not opposed to it in principle, but is there a version of this that works? That link says "tl;dr: It does not work." I don't want to develop custom tooling. This project is about moving away from that!
I don't expect so. There aren't that many PRs large enough to benefit from this technique, and large PRs tend to enter the codebase slowly.
Oh yeah... thanks!
You're welcome!
On September 15, 2020 at 11:04:45 PM, Tom Most (twm@freecog.net <mailto:twm@freecog.net>) wrote:
If you dig around you can see that there are various ways to make it work, including deploying the slash-command version rather than the on-push version.
I don't want to develop custom tooling. This project is about moving away from that!
At least if we're building new tooling we could build it separately from Twisted so it could apply across different projects; at least different projects in the org, a the very least ;)
I'm not usually a huge fan of rebasing, but presumably some history-rewriting could help avoid needing this?
On Monday, 14 September 2020 07:45:43 BST Tom Most wrote:
Hi all,
Craig and I have been working to land a switch to The *Black* Coding Style <https://black.readthedocs.io/en/stable/the_black_code_style.html>. It's in! The tree has been reformatted <https://github.com/twisted/twisted/commit/bc96c774be6e307e8e3e4d39780d37b045...>. Some notable changes:
How does this work with backwards compatibility? There are lots of symbols are that not black right? Barry
Happy to hear we're making progress on this! I'm the tiniest bit grumpy that this went forward without having the follow-up work already landed or ready-to-land simultaneously — my experience suggests that the fact that this is now complete means that neither you nor Craig will ever contribute to Twisted, or in fact ever answer your email, ever again — but if we can get it all landed promptly I will be over the moon :-).
Could we possibly use something like this: https://github.com/cclauss/autoblack <https://github.com/cclauss/autoblack> to just do the formatting rather than "check" if it's correct? PR templates are all well and good but the best checklist item is the one that's already checked off...
Can we please get this bulleted list in the form of a tool that people can run against a PR, so we don't need everyone to write their own bespoke shell script for fixing up their Twisted PRs? This is the thing I'm the most worried about - I'd like to have it done automatically because I'm seriously worried about work in progress getting abandoned because somebody backs themselves into a corner with an apparently intractable mega-conflict by not knowing how to properly use Git to apply this sort of modification. -g
On 2020-09-14 13:43, Glyph wrote:
It might be worthwhile but I think there is some downside to CI frequently injecting commits (or amending them).
Is .git-blame-ignore-revs going to be the next 'newsfile' what with every PR adding lines to it? Also, for the record, this doesn't address GitHub blame and does require everyone set the previously mentioned git config since it doesn't go with the repo. :[ Though that's mostly an annoyance with git. Oh yeah... thanks! Cheers, -kyle
On Tue, Sep 15, 2020, at 7:26 AM, Kyle Altendorf wrote:
I'm not opposed to it in principle, but is there a version of this that works? That link says "tl;dr: It does not work." I don't want to develop custom tooling. This project is about moving away from that!
I don't expect so. There aren't that many PRs large enough to benefit from this technique, and large PRs tend to enter the codebase slowly.
Oh yeah... thanks!
You're welcome!
On September 15, 2020 at 11:04:45 PM, Tom Most (twm@freecog.net <mailto:twm@freecog.net>) wrote:
If you dig around you can see that there are various ways to make it work, including deploying the slash-command version rather than the on-push version.
I don't want to develop custom tooling. This project is about moving away from that!
At least if we're building new tooling we could build it separately from Twisted so it could apply across different projects; at least different projects in the org, a the very least ;)
I'm not usually a huge fan of rebasing, but presumably some history-rewriting could help avoid needing this?
On Monday, 14 September 2020 07:45:43 BST Tom Most wrote:
Hi all,
Craig and I have been working to land a switch to The *Black* Coding Style <https://black.readthedocs.io/en/stable/the_black_code_style.html>. It's in! The tree has been reformatted <https://github.com/twisted/twisted/commit/bc96c774be6e307e8e3e4d39780d37b045...>. Some notable changes:
How does this work with backwards compatibility? There are lots of symbols are that not black right? Barry
participants (4)
-
Barry Scott -
Glyph -
Kyle Altendorf -
Tom Most