Use black for python codestyle #386

willingc · 2018-06-11T02:30:05Z

There are only a few Python files in the devguide repo. I've run them through the Black codestyle formatter for easier readability.

Mariatta

Looks great! Thanks!

serhiy-storchaka

The configuration file has a special purpose. Most changes it it don't look enhancements to me.

serhiy-storchaka · 2018-06-11T05:20:15Z

conf.py

@@ -18,14 +18,14 @@
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.
-#sys.path.insert(0, os.path.abspath('.'))
+# sys.path.insert(0, os.path.abspath('.'))


I'm not sure this is an enhancement.

This is not a human-readable comment as above lines, but a commented out sample. Currently you need to press DEL only once for switching it on. With your changes you will need to press DEL twice, and it will be harder to distinguish it from the above comment.

serhiy-storchaka · 2018-06-11T05:22:21Z

conf.py

@@ -111,61 +111,61 @@
 html_sidebars = {
    # Defaults taken from http://www.sphinx-doc.org/en/stable/config.html#confval-html_sidebars
    # Removes the quick search block
-    '**': ['localtoc.html', 'globaltoc.html', 'relations.html', 'customsourcelink.html'],
+    '**': ['localtoc.html', 'globaltoc.html', 'relations.html', 'customsourcelink.html']


Trailing comma makes the dict easier for modification.

and hence is considered good practice.

serhiy-storchaka · 2018-06-11T05:24:57Z

conf.py

+        u'Python Developer\'s Guide Documentation',
+        u'Brett Cannon',
+        'manual',
+    )


Trailing comma makes the list easier for modification.

And I'm not sure that writing 5-tuple vertically adds readability (especially if add yet few 5-tuples) .

tools/rstlint.py

serhiy-storchaka · 2018-06-11T05:29:08Z

tools/rstlint.py



 def main(argv):
-    usage = '''\
+    usage = (


What is the purpose of this change?

I presume to have a closing ')' that lines up with 'usage' and 'try', as in some C styles with '}'. But unneeded (...) around Python expressions is not typical style.

Going to change this back to the original which I agree is cleaner.

Multiline strings are one of the two places in the Python grammar where significant indentation is broken. Black tries to avoid unnecessary parentheses but is easily fooled with multiline strings that aren't aligned with the current indentation level. This is a technical limitation more than conscious design.

ncoghlan · 2018-06-11T12:01:41Z

I think I'm with Serhiy here: the utility of code formatters like black is to handle cases where the formatting really doesn't matter, and there's money to be saved by getting your developers to spend less time arguing about trivialities in a code review.

I don't think "the formatting doesn't really matter" holds in the case of documentation examples - you'll often want to format things in particular ways in order to highlight specific lines and constructs, and that formatting may not match what you might do in "real" code.

terryjreedy · 2018-06-11T14:24:14Z

conf.py

@@ -18,14 +18,14 @@
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.
-#sys.path.insert(0, os.path.abspath('.'))
+# sys.path.insert(0, os.path.abspath('.'))


terryjreedy · 2018-06-11T14:34:41Z

conf.py

@@ -111,61 +111,61 @@
 html_sidebars = {
    # Defaults taken from http://www.sphinx-doc.org/en/stable/config.html#confval-html_sidebars
    # Removes the quick search block
-    '**': ['localtoc.html', 'globaltoc.html', 'relations.html', 'customsourcelink.html'],
+    '**': ['localtoc.html', 'globaltoc.html', 'relations.html', 'customsourcelink.html']


and hence is considered good practice.

terryjreedy · 2018-06-11T14:38:58Z

tools/rstlint.py

@@ -221,8 +312,10 @@ def main(argv):
    else:
        for severity in sorted(count):
            number = count[severity]
-            print('%d problem%s with severity %d found.' %
-                  (number, number > 1 and 's' or '', severity))


This is a standard way to write this.

We follow PEP 8's "operators go in the front" revision from 2016. All code everywhere goes against this style because even PEP 8 used to advise the other way round. pycodestyle even has a warning about this (W503). I don't feel string interpolation is special enough to break the rules.

terryjreedy · 2018-06-11T14:42:39Z

tools/rstlint.py



 def main(argv):
-    usage = '''\
+    usage = (


I presume to have a closing ')' that lines up with 'usage' and 'try', as in some C styles with '}'. But unneeded (...) around Python expressions is not typical style.

tools/rstlint.py

willingc · 2018-06-11T15:44:41Z

Hi folks,

Thanks for the review and the comments. I expected there to be opinions on these changes since it is style ;-)

A few general comments:

I wanted to see responses to Black's default style (with the exception of changing string format for single or double quotes). This seems like a low risk repo to test the code style as well as getting feedback from all of you. cc/ @ambv
As this is one of the repos that new contributors get started on, I erred on the side of scanning readability of a file over conveniences of one or two keystrokes.
I am happy to go back and edit these files by hand for changes that folks feel strongly about.
I'll address individual comments later today.

Thanks all 😄

serhiy-storchaka · 2018-06-11T15:54:47Z

tools/rstlint.py

-                  (number, number > 1 and 's' or '', severity))
+            print(
+                '%d problem%s with severity %d found.'
+                % (number, number > 1 and 's' or '', severity)


I would rewrite number > 1 and 's' or '' as 's' if number > 1 else ''.

I agree 👍

ambv · 2018-06-12T02:17:49Z

FWIW removing trailing commas from indented single-element collection literals is a bug: psf/black#274 I will have it fixed for the next release.

willingc · 2018-06-12T02:19:04Z

Thanks folks for the review. I've gone back and made most of the suggested changes by @terryjreedy and @serhiy-storchaka.

serhiy-storchaka · 2018-06-12T05:12:33Z

tools/rstlint.py

@@ -138,6 +226,7 @@ def main(argv):
          -s sev   only show problems with severity >= sev
          -i path  ignore subdir or file path
 '''% argv[0]


It is worth to add a space before %.

serhiy-storchaka · 2018-06-12T05:21:14Z

conf.py

@@ -29,8 +29,8 @@

 # Add any Sphinx extension module names here, as strings. They can be extensions
 # coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
-extensions = ['sphinx.ext.intersphinx', 'sphinx.ext.todo']
-intersphinx_mapping = {'python': ('https://docs.python.org/3', None)}
+extensions = ['sphinx.ext.intersphinx', 'sphinx.ext.todo',]


Trailing comma has a value when the list is aligned vertically (and the trailing comma is followed by a newline instead of ]). You can easy comment out or copy-and-edit the last line by pressing just a few keys in many editors. But if the list is written in a single line, the trailing comma rather looks distractive to me.

willingc · 2018-06-16T18:28:31Z

@terryjreedy and @serhiy-storchaka I've made all recommended changes from your review with the exception of one place where we were in split agreement. If I don't hear any objection in a couple of days, I'm going to go ahead and merge. Thanks for the reviews:sunny:

serhiy-storchaka · 2018-06-16T18:31:45Z

My comment was related actually to all newly added trailing commas.

ezio-melotti

Misc style bikeshedding.

ezio-melotti · 2018-09-11T17:02:29Z

conf.py

+        u"Python Developer's Guide Documentation",
+        [u'Brett Cannon'],
+        1,
+    ),
 ]


Any reason to keep the u prefix around, especially since it's only used on some of the strings?
I would also put the square brackets and parentheses on the same line (i.e. [( and )]) and remove a level of indentation.

Black doesn't hug multiple levels of brackets to make it trivial to scan logical nesting levels visually.

The main issue is the fact that 3 lines are "wasted" for parentheses/brackets. The argument is also similar to the one I made below. I would either use:

man_pages = [('index', 'pythondevelopersguide', u"Python Developer's Guide Documentation", [u'Brett Cannon'], 1)]

This takes 1/3 of vertical space compared to the "expanded" version (but I would only use it in case of a single tuple) or

man_pages = [ ('index', 'pythondevelopersguide', u"Python Developer's Guide Documentation", [u'Brett Cannon'], 1), ]

This takes roughly half of the vertical space, and I would use it if I want/need more horizontal space or have (or planning to add) more tuples.

It also depends on the context: if it's a module-level constant I can splurge a bit and waste more vertical space, if it's a variable or a function call in the middle of a 10-lines function, I would avoid making the function twice as long if possible. The fact that the style is context-dependent is also the reason why I generally avoid using tools that alter the formatting I used.

tools/rstlint.py

Changes made.

@serhiy-storchaka

* blacken python files * add badge for codestyle * make all dicts and lists consistent with a trailing comma * edits per @serhiy-storchaka and @terryjreedy review * Edit per @serhiy-storchaka review * Edit per @serihy-storchaka clarification * Remove unneeded unicode u

the-knights-who-say-ni added the CLA signed label Jun 11, 2018

willingc requested review from brettcannon and Mariatta June 11, 2018 02:30

Mariatta approved these changes Jun 11, 2018

View reviewed changes

serhiy-storchaka reviewed Jun 11, 2018

View reviewed changes

terryjreedy previously requested changes Jun 11, 2018

View reviewed changes

serhiy-storchaka reviewed Jun 11, 2018

View reviewed changes

serhiy-storchaka reviewed Jun 12, 2018

View reviewed changes

brettcannon removed their request for review July 20, 2018 23:20

ezio-melotti reviewed Sep 11, 2018

View reviewed changes

willingc added 6 commits September 14, 2018 08:02

blacken python files

d3f0af8

add badge for codestyle

b1f807d

make all dicts and lists consistent with a trailing comma

e372963

edits per @serhiy-storchaka and @terryjreedy review

6265e0c

Edit per @serhiy-storchaka review

f021ebb

Edit per @serihy-storchaka clarification

347da02

willingc force-pushed the blacken-py branch from 58f6470 to 347da02 Compare September 14, 2018 15:06

Remove unneeded unicode u

681eb8e

willingc merged commit 287df5f into python:master Sep 14, 2018

willingc deleted the blacken-py branch September 14, 2018 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use black for python codestyle #386

Use black for python codestyle #386

willingc commented Jun 11, 2018

Mariatta left a comment

serhiy-storchaka left a comment

serhiy-storchaka Jun 11, 2018

terryjreedy Jun 11, 2018

serhiy-storchaka Jun 11, 2018

terryjreedy Jun 11, 2018

serhiy-storchaka Jun 11, 2018

serhiy-storchaka Jun 11, 2018

terryjreedy Jun 11, 2018

willingc Jun 12, 2018

ambv Jun 12, 2018

ncoghlan commented Jun 11, 2018

terryjreedy Jun 11, 2018

terryjreedy Jun 11, 2018

terryjreedy Jun 11, 2018

ambv Jun 12, 2018

terryjreedy Jun 11, 2018

willingc commented Jun 11, 2018

serhiy-storchaka Jun 11, 2018

willingc Jun 12, 2018

ambv commented Jun 12, 2018

willingc commented Jun 12, 2018

serhiy-storchaka Jun 12, 2018

serhiy-storchaka Jun 12, 2018

willingc commented Jun 16, 2018

serhiy-storchaka commented Jun 16, 2018

ezio-melotti left a comment

ezio-melotti Sep 11, 2018

ambv Sep 11, 2018

ezio-melotti Sep 11, 2018

Use black for python codestyle #386

Use black for python codestyle #386

Conversation

willingc commented Jun 11, 2018

Mariatta left a comment

Choose a reason for hiding this comment

serhiy-storchaka 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan commented Jun 11, 2018

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

willingc commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambv commented Jun 12, 2018

willingc commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc commented Jun 16, 2018

serhiy-storchaka commented Jun 16, 2018

ezio-melotti 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