[9512] Initial configuration for black, take two #1380

twm · 2020-09-12T06:26:35Z

This is a rebase of, and closes #1134. I removed the documentation changes that proved so contentious, but added a few necessary to reconcile with The Black Code Style.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9512
The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
I have updated the automated tests.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

See https://github.com/psf/black/blob/master/docs/the_black_code_style.md

rodrigc

The old pycodestyle target would fail in CI if someone submitted a PR,
but it did not pass pycodestyle changes.

If someone submits a PR, and it is not formatted with black,
will there be a CI failure, so that the user will know that they have to reformat their patch with black,
with tox -e black?

rodrigc · 2020-09-12T18:11:36Z

tox.ini

+
+[testenv:black]
+skip_install=True
+basepython=python3.6


In this PR, the basepython for running black is Python 3.6.
However, Python 3.5 support has not yet been dropped from Twisted.

Do you need to specify --target to black, to prevent style changes incompatible with Python 3.5 from going in?

I'm not in favor of pinning the tox environment for black to a specific Python version,
because a Twisted contributor may have a different version of Python installed on their system,
and tox -e black will not work for them.

I believe that the target-version in pyproject.toml ensures that 3.5-compatible syntax is used.

I think you are correct. That's good enough then.

rodrigc · 2020-09-12T18:18:41Z

tox.ini

@@ -151,6 +148,25 @@ deps=https://github.com/twisted/pydoctor/archive/3f9c64829dfa040b334c9ae27c332c7
 [testenv:manifest-checker]
 skip_install = true

+[testenv:lint]
+skip_install=True
+basepython=python3.6


I'm not in favor of pinning the lint target to Python 3.6,
because tox -e lint will not work for contributors who do not have Python 3.6 installed.

I don't really understand the motivation for this change myself. Perhaps the goal was to prevent linting tools from running under Python 2.7 if Tox was installed with that Python version? I'll remove it.

rodrigc · 2020-09-12T18:20:30Z

tox.ini

-    lint: diff-cover==0.9.12
-    lint: pycodestyle
+
+    black: black>=19.3b0


Further down in the deps section, you have: black==20.8b1.
Might be good to keep this consistent

Thanks, I'll clean this up.

rodrigc

Reference to pycodestyle should be removed from README.rst.

Might want to add reference to tox -e black there.

rodrigc · 2020-09-12T20:58:40Z

If a user files a PR, and their PR does not match black's coding style,
how will they, or a code reviewer know that they need to run tox -e black to fix things up?

twm · 2020-09-12T22:16:10Z

@rodrigc Thank you for the review! I believe that I have addressed all your comments.

Changes

I consolidated the Tox configuration to have a factor-based style throughout. This seemed to be most similar to what was already there. I'd contrast the factor-based style with the explicit [testenv:foo] style used in Klein. I did lift the [default] section technique for variables from Klein, though.

I removed some detritus for a pyflakes Tox environment. There is also some stuff for a wheel factor (not the same as the wheels factor!), but I'm not sure enough about that to remove it, and that's really out of scope for this PR, anyway.

I added a GitHub Actions workflow that runs the linting tools. I like GHA for this because it is lower-latency than Travis, though the log UI is more difficult to use. I also filed #9953 to do more of this.

Merge Procedure

There is a bit of instability in Black's output on one file that'll need to be worked around when running it (filed upstream here), so the procedure on merge will be:

tox -e black-reformat -- --fast src/twisted/words/test/test_irc.py
tox -e black-reformat -- --fast src/twisted/words/test/test_irc.py
tox -e black-reformat

Running the tool twice with --fast is the workaround suggested in the Black ticket I linked. I have verified that it works for this file.

Overall:

Run Black (as above)
Commit the result; push to trunk. No review is required because it is the output of a tool.
Create a PR that adds the above commit to .git-blame-ignore-revs and makes the lint GHA build required.

rodrigc · 2020-09-13T00:23:50Z

README.rst

+
+To automatically format code according to the coding standards::
+
+  $ tox -e black-reformat


Nice, this gives clear instructions on how to reformat the code.

rodrigc · 2020-09-13T00:26:31Z

docs/core/development/policy/coding-standard.rst

@@ -76,18 +76,19 @@ Indentation is 4 spaces per indent.
 Tabs are not allowed.
 It is preferred that every block appears on a new line, so that control structure indentation is always visible.

-Lines are flowed at 79 columns.
+Lines are flowed at 88 columns per `The Black Code Style <https://github.com/psf/black/blob/master/docs/the_black_code_style.md>`_.


This is OK. In future, we may want to remove all mention of number of columns, etc.
from this document, and just say that Twisted follows the style guidelines as followed by Black, and just give a pointer Black's guidelines. In the end, very few people will read this anyways.

As long as they are told during review time to run tox -e black-reformat to make their
PR acceptable, that is probably good enough.

rodrigc · 2020-09-13T00:37:47Z

Might want to add the required invocation of black to the wiki here: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted

rodrigc · 2020-09-13T00:39:02Z

.github/workflows/lint.yaml

+
+    runs-on: ubuntu-20.04
+    env:
+      TOXENV: "lint,black,manifest-checker,twine"


Adding this GitHub action which invokes black is good.

rodrigc · 2020-09-13T00:42:54Z

tox.ini

@@ -148,20 +165,10 @@ commands =
 [testenv:apidocs]
 deps=https://github.com/twisted/pydoctor/archive/3f9c64829dfa040b334c9ae27c332c7078356e79.zip

-[testenv:manifest-checker]


OK, looks like this block which got deleted moved to: https://github.com/twisted/twisted/pull/1380/files#diff-b91f3d5bd63fcd17221b267e851608e8R108

rodrigc

I think this looks ok.

You have a GitHub Action which invokes black, so we can get feedback from CI
tox -e black runs the check, and tox -e black-reformat can be run by a contributor to reformat their code

So I would say merge this, and let's get this going.

In follow-up items, I would say:

Make a follow-up PR which reformats the tree. I can quickly review/approve
Make a follow-up PR which adds a value to .git-blame-ignore-revs. I can quickly review/approve
Send a e-mail to the mailing list, informing that coding style has changed to black, and give pointers to what a user should do to make their existing PR's acceptable to the new coding style.
The docs regarding coding style should be cut to the bare minimum, and just have pointers to the Black guidelines. Also, just mention in the docs that tox -e black-reformat must be run to make any contribution acceptable.
Take a pass through the wiki, and add references to black as appropriate, such as: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted
Do we need to add Black to the Contributor Checklist: that is displayed for ever PR?

hawkowl and others added 9 commits September 11, 2020 22:29

change coding standard docs

07c3ed5

add pyproject toml settings for black

9ca3f13

we don't need this anymore

9295d7e

black and update lint

a9ae0d2

changelog

e516b69

target version

4ef70c9

bump version

61c86a5

superfluous

99ee85b

Set isort line length to match Black

eab9055

See https://github.com/psf/black/blob/master/docs/the_black_code_style.md

twm force-pushed the 9512-black-v2 branch from 838e19f to 86014bc Compare September 12, 2020 06:52

Reconcile coding standard with Black and isort

a3d6d4c

twm force-pushed the 9512-black-v2 branch from 86014bc to a3d6d4c Compare September 12, 2020 06:54

Use the latest Black

4885381

twm marked this pull request as ready for review September 12, 2020 07:00

rodrigc reviewed Sep 12, 2020

View reviewed changes

rodrigc suggested changes Sep 12, 2020

View reviewed changes

twm added 2 commits September 12, 2020 15:10

More Tox configuration, now with documentation!

29d6c61

Add lint GitHub Actions workflow

0c8a31f

twm force-pushed the 9512-black-v2 branch from 8621f87 to 0c8a31f Compare September 12, 2020 22:14

Don't fail the build

6f87467

twm force-pushed the 9512-black-v2 branch from c68f13a to 6f87467 Compare September 12, 2020 22:33

rodrigc reviewed Sep 13, 2020

View reviewed changes

rodrigc approved these changes Sep 13, 2020

View reviewed changes

twm merged commit c5fbd88 into trunk Sep 13, 2020

twm deleted the 9512-black-v2 branch September 13, 2020 05:06

twm mentioned this pull request Sep 13, 2020

[9954] Fade to black #1381

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9512] Initial configuration for black, take two #1380

[9512] Initial configuration for black, take two #1380

twm commented Sep 12, 2020 •

edited

rodrigc left a comment

rodrigc Sep 12, 2020

twm Sep 12, 2020

rodrigc Sep 12, 2020

rodrigc Sep 12, 2020

twm Sep 12, 2020

rodrigc Sep 12, 2020

twm Sep 12, 2020

rodrigc left a comment

rodrigc commented Sep 12, 2020

twm commented Sep 12, 2020 •

edited

rodrigc Sep 13, 2020

rodrigc Sep 13, 2020

rodrigc commented Sep 13, 2020

rodrigc Sep 13, 2020

rodrigc Sep 13, 2020

rodrigc left a comment


		To automatically format code according to the coding standards::

		$ tox -e black-reformat

[9512] Initial configuration for black, take two #1380

[9512] Initial configuration for black, take two #1380

Conversation

twm commented Sep 12, 2020 • edited

Contributor Checklist:

rodrigc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc left a comment

Choose a reason for hiding this comment

rodrigc commented Sep 12, 2020

twm commented Sep 12, 2020 • edited

Changes

Merge Procedure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc left a comment

Choose a reason for hiding this comment

twm commented Sep 12, 2020 •

edited

twm commented Sep 12, 2020 •

edited