Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Use black for python codestyle #386
Conversation
|
Looks great! Thanks! |
|
The configuration file has a special purpose. Most changes it it don't look enhancements to me. |
| @@ -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('.')) | |||
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.
| @@ -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'] | |||
| u'Python Developer\'s Guide Documentation', | ||
| u'Brett Cannon', | ||
| 'manual', | ||
| ) |
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) .
|
|
||
|
|
||
| def main(argv): | ||
| usage = '''\ | ||
| usage = ( |
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.
willingc
Jun 12, 2018
Author
Collaborator
Going to change this back to the original which I agree is cleaner.
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.
|
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. |
| @@ -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('.')) | |||
| @@ -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'] | |||
| @@ -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)) | |||
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 = ( |
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.
|
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:
Thanks all |
| (number, number > 1 and 's' or '', severity)) | ||
| print( | ||
| '%d problem%s with severity %d found.' | ||
| % (number, number > 1 and 's' or '', severity) |
serhiy-storchaka
Jun 11, 2018
Member
I would rewrite number > 1 and 's' or '' as 's' if number > 1 else ''.
|
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. |
|
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] | |||
| @@ -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',] | |||
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.
|
@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 |
|
My comment was related actually to all newly added trailing commas. |
|
Misc style bikeshedding. |
| u"Python Developer's Guide Documentation", | ||
| [u'Brett Cannon'], | ||
| 1, | ||
| ), | ||
| ] |
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.
ambv
Sep 11, 2018
Contributor
Black doesn't hug multiple levels of brackets to make it trivial to scan logical nesting levels visually.
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.
There are only a few Python files in the devguide repo. I've run them through the Black codestyle formatter for easier readability.