[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/bc96c774be6e307e8e3e4d39780d37b045d0973a. 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
On Sep 13, 2020, at 11:45 PM, Tom Most twm@freecog.net 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/bc96c774be6e307e8e3e4d39780d37b045d0973a. Some notable changes:
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 :-).
- 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-revsThere are two new Tox testenvs. To check style:
tox -e blackTo adjust the formatting:
tox -e black-reformatThe formatting is checked by a new GitHub Actions lint built.
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...
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.
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:
On Sep 13, 2020, at 11:45 PM, Tom Most twm@freecog.net wrote: To adjust the formatting:
tox -e black-reformat
The formatting is checked by a new GitHub Actions lint built.
Could we possibly use something like this:
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...
It might be worthwhile but I think there is some downside to CI frequently injecting commits (or amending them).
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.
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:
On 2020-09-14 13:43, Glyph wrote:
On Sep 13, 2020, at 11:45 PM, Tom Most twm@freecog.net wrote: To adjust the formatting:
tox -e black-reformat
The formatting is checked by a new GitHub Actions lint built.
Could we possibly use something like this:
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...
It might be worthwhile but I think there is some downside to CI frequently injecting commits (or amending them).
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!
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.
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.
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:
On Tue, Sep 15, 2020, at 7:26 AM, Kyle Altendorf wrote:
On 2020-09-14 13:43, Glyph wrote:
On Sep 13, 2020, at 11:45 PM, Tom Most <twm@freecog.net mailto:twm@freecog.net> wrote: To adjust the formatting:
tox -e black-reformat
The formatting is checked by a new GitHub Actions lint built.
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...
It might be worthwhile but I think there is some downside to CI frequently injecting commits (or amending them).
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."
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 ;)
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.
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.
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.
I'm not usually a huge fan of rebasing, but presumably some history-rewriting could help avoid needing this?
Oh yeah... thanks!
You're welcome!
Twisted-Python mailing list Twisted-Python@twistedmatrix.com mailto:Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
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/bc96c774be6e307e8e3e4d39780d37b045d0973a. Some notable changes:
How does this work with backwards compatibility? There are lots of symbols are that not black right?
Barry
- 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-revsThere are two new Tox testenvs. To check style:
tox -e blackTo adjust the formatting:
tox -e black-reformatThe 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
On Sep 15, 2020, at 10:08 AM, Barry Scott barry.scott@forcepoint.com wrote:
How does this work with backwards compatibility? There are lots of symbols are that not black right?
PEP8 suggests (but does not mandate) snake_case identifiers where Twisted uses camelCase. But Black doesn't care at all about identifiers; its job is more or less exclusively about formatting whitespace. So there's no compatibility concern here.
-g
participants (4)
-
Barry Scott -
Glyph -
Kyle Altendorf -
Tom Most