[#9512] Initial configuration for black #1134

hawkowl · 2019-05-05T19:53:05Z

https://twistedmatrix.com/trac/ticket/9512

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

glyph · 2019-05-08T21:04:44Z

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

@@ -229,6 +230,8 @@ Docstrings should always be used to describe the purpose of methods, functions,
 Moreover, all methods, functions, classes, and modules must have docstrings.
 In addition to documenting the purpose of the object, the docstring must document all of parameters or attributes of the object.

+Docstrings must be reflowed at 79 characters.


If someone could turn https://github.com/glyph/python-docstring-mode/blob/master/docstring_wrap.py into a tool that ran over a codebase's docstrings rather than just an editor plugin shim helper gizmo, we could possibly Black-ify our epytext as well...

Perhaps file a ticket against Black for this?

This docstring wrapper makes a number of assumptions about the format of the docstrings it's wrapping, most of which are going to be wrong for a lot of projects. I don't think there's any way to realistically make it universal enough for Black to consider incorporating it.

Are the assumptions specific to Twisted or specific to particular docstring formats? In the latter case, being able to specify the docstring format to Black and/or supporting the __docformat__ constant could be used to turn the docstring reformatting on/off.

wsanchez · 2019-12-10T19:54:26Z

pyproject.toml

@@ -50,3 +50,6 @@
        directory = "misc"
        name = "Misc"
        showcontent = false
+
+[tool.black]
+skip-string-normalization = true


If this is because u"" is getting changed to "", I think that passing --target-version=py27 might be a more appropriate way to get there.

wsanchez · 2019-12-10T19:56:25Z

tox.ini

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



FWIW I found it useful to add a test rnv black-reformat to that it's easy for a developer to reformat code to comply with black. Example here.

wsanchez · 2019-12-10T19:59:25Z

Are we going to toss out the 3-newline-before-class thing when we start using black or is there a way to keep that?

twm

All of the blocker comments here seem to have been addressed. Let's end the petty tyranny of twistedchecker forever. 🎉

twm · 2020-07-12T18:36:08Z

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

@@ -229,6 +230,8 @@ Docstrings should always be used to describe the purpose of methods, functions,
 Moreover, all methods, functions, classes, and modules must have docstrings.
 In addition to documenting the purpose of the object, the docstring must document all of parameters or attributes of the object.

+Docstrings must be reflowed at 79 characters.


Perhaps file a ticket against Black for this?

glyph · 2020-07-15T07:47:58Z

Are we going to toss out the 3-newline-before-class thing when we start using black or is there a way to keep that?

We're tossing it; the appeal of adopting Black in the first place is that we will diverge from common practice in fewer situations.

glyph

I have three blockers here that I'd like to see resolved before landing this:

Let's please move out all the unrelated documentation changes so they're not blocked behind my other two blockers, and...
I actively do not want anyone running black over files in the Twisted codebase until we have a documented solution for what to do with existing PRs. I want to make sure that when we run it for the first time, we do not ruin Git Blame forever, which means we need a dedicated point in history where the formatting occurs, not a scattershot format-as-you-go approach.
We should also have a tooling to run over an existing PR to ensure that everyone with an open branch (of which there are presently 1853 on origin alone) doesn't need to manually reconstruct all of their work. Over on Trac, Hawkowl says:

I also believe that if you merge in this merge commit, blackify your PR, then merge trunk, git should know what to do.

If by "blackify your PR, then merge trunk" you mean "add a commit that runs black", when I try that I see big irrelevant conflicts, so at least this isn't entirely obvious. But perhaps there's some difference in configuration? Or maybe you meant something different about using history rewriting to make each change have black run over it on its own? Can someone write down the specific commands to run, then verify that they work across some existing PRs?

If we squash merge PRs, then we'll also get a nice history for those PRs.

We don't rewrite history currently and squash merges typically make this sort of problem worse, not better.

glyph · 2020-07-15T07:48:52Z

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

@@ -434,28 +419,6 @@ For example scripts you expect a Twisted user to run from the command-line, add
    #!/usr/bin/env python


-Standard Library Extension Modules


Can we do this doc update in a different PR? It doesn't have anything to do with Black.

glyph · 2020-07-15T07:49:03Z

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

@@ -477,15 +440,6 @@ For example, a Service subclass for Forums might be named ``twisted.forum.servic
 Since neither of these modules are volatile *(see above)* the classes may be imported directly into the user's namespace and not cause confusion.


-New-style Classes


This doesn't seem to have anything to do with Black either.

glyph · 2020-07-15T07:49:12Z

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

@@ -618,17 +572,6 @@ An attribute (or function, method or class) should be considered private when on
 - The attribute is part of a known-to-be-sub-optimal interface and will certainly be removed in a future release.


-Python 3


This also doesn't seem to have anything to do with Black

glyph · 2020-07-15T07:49:26Z

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

@@ -644,10 +587,10 @@ All SQL keywords should be in upper case.
 C Code
 ------

-C code must be optional, and work across multiple platforms (MSVC++9/10/14 for Pythons 2.7 and 3.5+ on Windows, as well as recent GCCs and Clangs for Linux, macOS, and FreeBSD).
+C code must be optional, and work across multiple platforms (MSVC++14 for Python 3.5+ on Windows, as well as recent GCCs and Clangs for Linux, macOS, and FreeBSD).


Not related to this change

glyph · 2020-07-15T07:49:32Z

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


 C code should be kept in external bindings packages which Twisted depends on.
-If creating new C extension modules, using `cffi <https://cffi.readthedocs.io/en/latest/>`_ is highly encouraged, as it will perform well on PyPy and CPython, and be easier to use on Python 2 and 3.
+If creating new C extension modules, using `cffi <https://cffi.readthedocs.io/en/latest/>`_ is highly encouraged, as it will perform well on PyPy and CPython.


Not related to this change

glyph · 2020-07-15T07:51:09Z

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

-All strings in Twisted which are not interfacing directly with Python (e.g. ``sys.path`` contents, module names, and anything which returns ``str`` on both Python 2 and 3)  should be marked explicitly as "bytestrings" or "text/Unicode strings".
-This is done by using the ``b`` (for bytestrings) or ``u`` (for Unicode strings) prefixes when using string literals.
-String literals not marked with this are "native/bare strings", and have a different meaning on Python 2 (where a bare string is a bytestring) and Python 3 (where a bare string is a Unicode string).
+All literal strings in Twisted which contain binary data must be prefixed with ``b`` (for bytestring).


This doesn't seem related to this change, but also, do we need to even say this any more? This seems to be just a basic Python syntax fact now.

glyph · 2020-07-15T08:10:25Z

(I should note that the best option here is not to document how someone can fix up their own work, but just to write a script that pushes a Black-fixup commit to every open PR; as long as we hit most of the open branches, it's fine; having to fix up one or two PRs or branches manually out of ~thousands is not too high a price to pay.)

mthuurne · 2020-07-22T02:35:00Z

I'm attempting to add type annotations by automated replacements on the source tree, but the code style checker is a bottleneck there: even if the automated replacement doesn't break the rules, it can be rejected by the style checker if the replacement was close to preexisting non-compliant code. To correct these manually is too time consuming for some replacements, so I have to shelve those ideas until Black adoption has been merged.

Of course I'm a bit biased because of the above, but I don't think breaking PRs should be a blocker:

there are currently 65 open PRs
40 open PRs are over a year old and will likely require rework even without considering Black
the addition of type annotations will also cause PRs to require rework:
- PRs will need to pass mypy checks before they can get merged
- sometime soon we should start to require type annotations on new code, in my opinion

So all in all, there isn't a huge number of open PRs, but the ones that we do have will likely need manual updates anyway.

hawkowl and others added 11 commits May 5, 2019 15:39

change coding standard docs

8468042

add pyproject toml settings for black

baec613

do py36 for this

cf931b4

we don't need this anymore

ae05f80

black and update lint

b0153a0

cleanup

3a3baec

changelog

47e883e

fix travis

1196043

test

4b7eac5

Merge branch 'trunk' into 9512-black

6592490

Merge branch 'trunk' into 9512-black

340727b

glyph reviewed May 8, 2019

View reviewed changes

docs/core/development/policy/coding-standard.rst Show resolved Hide resolved

glyph reviewed May 8, 2019

View reviewed changes

wsanchez reviewed Dec 10, 2019

View reviewed changes

wsanchez and others added 8 commits December 10, 2019 13:12

Merge branch 'trunk' into 9512-black

fcccf1c

Merge remote-tracking branch 'origin/trunk' into 9512-black

2e964bd

tweak tox

7103a96

target version

f61b7f8

bump version

649181d

superfluous

880cbfd

Merge branch 'trunk' into 9512-black

4aa8e31

Merge branch 'trunk' into 9512-black

25a9dbb

twm approved these changes Jul 12, 2020

View reviewed changes

glyph requested changes Jul 15, 2020

View reviewed changes

twm mentioned this pull request Jul 18, 2020

[9917] Add .git-blame-ignore-revs placeholder #1356

Merged

5 tasks

mthuurne mentioned this pull request Jul 24, 2020

Disable pycodestyle for now #1365

Merged

5 tasks

twm mentioned this pull request Sep 12, 2020

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

Merged

5 tasks

Merge branch 'trunk' into 9512-black

6d28f74

twm closed this in #1380 Sep 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#9512] Initial configuration for black #1134

[#9512] Initial configuration for black #1134

hawkowl commented May 5, 2019 •

edited

Loading

glyph May 8, 2019

twm Jul 12, 2020

glyph Jul 15, 2020

mthuurne Jul 15, 2020

wsanchez Dec 10, 2019

wsanchez Dec 10, 2019

wsanchez Dec 10, 2019

wsanchez commented Dec 10, 2019

twm left a comment

twm Jul 12, 2020

glyph commented Jul 15, 2020 •

edited

Loading

glyph left a comment •

edited

Loading

glyph Jul 15, 2020

glyph Jul 15, 2020

glyph Jul 15, 2020

glyph Jul 15, 2020

glyph Jul 15, 2020

glyph Jul 15, 2020

glyph commented Jul 15, 2020

mthuurne commented Jul 22, 2020

		@@ -434,28 +419,6 @@ For example scripts you expect a Twisted user to run from the command-line, add
		#!/usr/bin/env python


		Standard Library Extension Modules

		@@ -477,15 +440,6 @@ For example, a Service subclass for Forums might be named ``twisted.forum.servic
		Since neither of these modules are volatile (see above) the classes may be imported directly into the user's namespace and not cause confusion.


		New-style Classes

		@@ -618,17 +572,6 @@ An attribute (or function, method or class) should be considered private when on
		- The attribute is part of a known-to-be-sub-optimal interface and will certainly be removed in a future release.


		Python 3

[#9512] Initial configuration for black #1134

[#9512] Initial configuration for black #1134

Conversation

hawkowl commented May 5, 2019 • edited Loading

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

wsanchez commented Dec 10, 2019

twm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Jul 15, 2020 • edited Loading

glyph left a comment • edited Loading

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

glyph commented Jul 15, 2020

mthuurne commented Jul 22, 2020

hawkowl commented May 5, 2019 •

edited

Loading

glyph commented Jul 15, 2020 •

edited

Loading

glyph left a comment •

edited

Loading