Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use black for python codestyle #386

Merged
merged 7 commits into from Sep 14, 2018
Merged

Use black for python codestyle #386

merged 7 commits into from Sep 14, 2018

Conversation

@willingc
Copy link
Collaborator

@willingc willingc commented Jun 11, 2018

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

Copy link
Member

@Mariatta Mariatta left a comment

Looks great! Thanks!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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

conf.py Outdated
@@ -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('.'))

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 11, 2018
Member

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.

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

I agree.

conf.py Outdated
@@ -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']

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 11, 2018
Member

Trailing comma makes the dict easier for modification.

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

and hence is considered good practice.

conf.py Outdated
u'Python Developer\'s Guide Documentation',
u'Brett Cannon',
'manual',
)

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 11, 2018
Member

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 Show resolved Hide resolved


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

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 11, 2018
Member

What is the purpose of this change?

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

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.

This comment has been minimized.

@willingc

willingc Jun 12, 2018
Author Collaborator

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

This comment has been minimized.

@ambv

ambv Jun 12, 2018
Contributor

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
Copy link
Contributor

@ncoghlan ncoghlan commented Jun 11, 2018

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.

conf.py Outdated
@@ -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('.'))

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

I agree.

conf.py Outdated
@@ -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']

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

and hence is considered good practice.

@@ -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 comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

This is a standard way to write this.

This comment has been minimized.

@ambv

ambv Jun 12, 2018
Contributor

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.



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

This comment has been minimized.

@terryjreedy

terryjreedy Jun 11, 2018
Member

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 Show resolved Hide resolved
@willingc
Copy link
Collaborator Author

@willingc willingc commented Jun 11, 2018

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 😄

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

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 11, 2018
Member

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

This comment has been minimized.

@willingc

willingc Jun 12, 2018
Author Collaborator

I agree 👍

@ambv
Copy link
Contributor

@ambv ambv commented Jun 12, 2018

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
Copy link
Collaborator Author

@willingc willingc commented Jun 12, 2018

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

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

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 12, 2018
Member

It is worth to add a space before %.

conf.py Outdated
@@ -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',]

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jun 12, 2018
Member

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
Copy link
Collaborator Author

@willingc willingc commented Jun 16, 2018

@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☀️

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jun 16, 2018

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

@brettcannon brettcannon removed their request for review Jul 20, 2018
Copy link
Member

@ezio-melotti ezio-melotti left a comment

Misc style bikeshedding.

conf.py Outdated
u"Python Developer's Guide Documentation",
[u'Brett Cannon'],
1,
),
]

This comment has been minimized.

@ezio-melotti

ezio-melotti Sep 11, 2018
Member

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.

This comment has been minimized.

@ambv

ambv Sep 11, 2018
Contributor

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

This comment has been minimized.

@ezio-melotti

ezio-melotti Sep 11, 2018
Member

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 Show resolved Hide resolved
tools/rstlint.py Show resolved Hide resolved
willingc added 6 commits Jun 11, 2018
@willingc willingc force-pushed the willingc:blacken-py branch from 58f6470 to 347da02 Sep 14, 2018
@willingc willingc dismissed terryjreedy’s stale review Sep 14, 2018

Changes made.

@willingc willingc merged commit 287df5f into python:master Sep 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willingc willingc deleted the willingc:blacken-py branch Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.