Closed grantjenks closed 3 years ago
Let's consider not squeezing hanging comments to the left, e.g. not doing this:
- def _coerce(self, value, default: Optional[int] = 0) -> Optional[int]: # type: ignore[no-untyped-def]
+ def _coerce(self, value, default: Optional[int] = 0) -> Optional[int]: # type: ignore[no-untyped-def]
And here's an egregious example from flufl.i18n:
- aqua = 'aardvarks' # noqa: F841
- blue = 'badgers' # noqa: F841
- cyan = 'cats' # noqa: F841
+ aqua = 'aardvarks' # noqa: F841
+ blue = 'badgers' # noqa: F841
+ cyan = 'cats' # noqa: F841
The latter is definitely less readable.
Here's one from flufl.lock that I'm not so sure about:
-#sys.path.append(os.path.abspath('.'))
+# sys.path.append(os.path.abspath('.'))
blue
should not de-parenthesize ternary operators. The parentheses aid in readability.
-SEP = ('^' if sys.platform == 'win32' else '|')
+SEP = '^' if sys.platform == 'win32' else '|'
I'm still not sure it's more readable to cozy up multiline argument lists into a single line:
def __init__(
- self,
- lockfile: str,
- lifetime: Optional[Interval] = None,
- separator: str = SEP
- ):
+ self, lockfile: str, lifetime: Optional[Interval] = None, separator: str = SEP
+ ):
I'm not sure it's more readable to de-cozy generator expressions:
- self._claimfile = self._separator.join((
- self._lockfile,
- self.hostname,
- str(os.getpid()),
- str(random.randint(0, MAXINT)),
- ))
+ self._claimfile = self._separator.join(
+ (
+ self._lockfile,
+ self.hostname,
+ str(os.getpid()),
+ str(random.randint(0, MAXINT)),
+ )
+ )
Okay, I get it, but I kind of don't like it 😄
@public
class Template(string.Template):
"""Match any attribute path."""
+
idpattern = r'[_a-z][_a-z0-9.]*'
Moved ideas to individual tickets ...
The exploding of blocks like these is one of the things that turn me away from black:
something = process_some_list([
abc, def, ghi,
])
I didn’t find a ticket for it!
Hi, thanks for blue. Here 3 suggestions:
"""
Example
"""
# 1.triple quoted non docstrings?
# 2. Function on them always line broken?
t = """
foo
""".split(
'bar'
)
def foo():
"""func doc""" # double quotes for docstrings => yes
s = 'hello world' # single quotes for code => yes
# again 1.: Always double apos?
stmt = """
SELECT *
FROM foo
WHERE bar;
"""
m = {'a': 'b'}
# 3. trailing comma & short line: Always line break?:
print(
42,
)
Suggested:
"""
Example
"""
t = '''
foo
'''.split('bar')
def foo():
"""func doc"""
s = 'hello world'
stmt = '''
SELECT *
FROM foo
WHERE bar;
'''
m = {'a': 'b'}
print(42,)
so:
Maybe introduce a CLI flag which allows to triple single quote non docstrings
and 3. Check before breaking if the line would fit into a single line and if so: Not break line.
and 3. I have in axblack, from which I forward now to this project (hope that's ok).
This:
def very_important_function(
template: str,
*variables,
file: os.PathLike,
debug: bool = False,
):
code
code
Should be formatted like this
def very_important_function(
template: str,
*variables,
file: os.PathLike,
debug: bool = False,
):
code
code
To comply with PEP-8 indentation directives:
# Add 4 spaces (an extra level of indentation) to distinguish arguments from the rest.
def long_function_name(
var_one, var_two, var_three,
var_four):
print(var_one)
The first one is ok, the third one better, but the second one isn’t, because the ):
is aligned with the next line even though it is a new block.
@merwok The dedent at the level of the def looks awful. You are already two levels up from the def. the closing parenthesis should be so that it does not visually bring you back to column zero. The next indentation level after the def line (even when split over multiple lines) should be one deeper compared to the def initial level. Having the ): at the same level brings you visually back to indentation zero when you should be ready to be at indentation one.
The reason why I don't like black is that it does the following:
- if not admin.modules.virtual(self.module) and not admin.modules.recognize(self.module, self.dn, self.oldattr):
- raise admin.uexceptions.wrongObjectType('%s is not recognized as %s.' % (self.dn, self.module))
+ if not admin.modules.virtual(
+ self.module
+ ) and not admin.modules.recognize(
+ self.module, self.dn, self.oldattr
+ ):
+ raise admin.uexceptions.wrongObjectType(
+ "%s is not recognized as %s." % (self.dn, self.module)
+ )
→ This is now worse to read. Splitting everything into multiple lines makes it slow to read and understand the concept of basically 2 statements.
- return [
- name
- for name in self.descriptions
- if name in event.args
- ]
+ return [name for name in self.descriptions if name in event.args]
→ And on the other side it puts this into one line, where readability might be better with the splitted multiline version.
@spaceone I would rewrite that as
if (not admin.modules.virtual(self.module)
and not admin.modules.recognize(self.module, self.dn, self.oldattr)):
raise admin.uexceptions.wrongObjectType('%s is not recognized as %s.' % (self.dn, self.module))
This way it keeps the two conditions separate and readable, and you indent one space more because otherwise it is confused with the next block, as from PEP-8 recommendation to add one more indent in that case. The problem is that it's really hard to automatise something like that, because it's purely a human decision on where to split multiple logical conditions.
In reality, what I would likely do, is to add supporting variables to shorten the if condition.
My suggestion for fixing the case of the list comprehensions is quite simple: never join lines. Not if they go under the column limit. Not ever. There should be a vertical style and a horizontal style. In black there is and you can force the vertical with a trailing comma, but that doesn't work for comprehensions. Elmformat has this vertical-or-horizontal rule based on the presence of a newline in the expression and it's great.
With this rule the list comprehension @spaceone shows above would be unchanged and you'd get this:
- return [name for name in self.descriptions if name in
- event.args]
+ return [
+ name
+ for name in self.descriptions
+ if name in event.args
+ ]
Which I think it pretty great. You want to switch to vertical for a comprehension, function definition, anything? Just hit enter randomly in it and reformat.
Never rejoining also reduces formatting churn on renaming of variables and such.
The single major advantage of Black
is that it removes the necessity of having to discuss something as trivial as style
. OTOH, I've always had a fundamental disagreement with them on their choice of quotes, so Blue
being on my side for that made it interesting.
But, having read this thread, I see that all this project is going to do is add the new question: what colour should the bikeshed be?
I guess there's always room for new opinions. Feck it, I've alway been a Goth at heart, so I'm going "Back to Black".
The single major advantage of
Black
is that it removes the necessity of having to discuss something as trivial asstyle
.
One could argue that teams should be free to set their own standards, and as long as they comply with PEP-8, there's no reason to argue. Personally, I believe that black should not exist at all. This package is just a knee jerk reaction to those who obey the church of Black, and I for one fully support it.
Not sure if you’re saying blue or black should not exist, and whether you fully support black or blue.
Not sure if you’re saying blue or black should not exist, and whether you fully support black or blue.
I support blue because black exists. If black didn't exist, I would still support blue for customisability, but my point is that there's a limit to how much you can argue about formatting. Black is pushing it too far. PEP-8 is enough, and if any developer has opinions that go beyond PEP-8 compliance, this developer should be simply told to shut up.
The number one reason why blue exists for me is black's choice of double quotes, which is an ergonomic disaster. The fact that black doesn't even really make this configurable is even worse. That said, I think the long term viability of both blue and black is questionable, given the existence of ruff. I'm hoping that once ruff implements autoformatting and gives us a choice of the default quoting character, the need for blue will decrease. It's been difficult to find the bandwidth and motivation to keep blue in sync with black, and I'd honestly rather contribute to ruff over the long term.
I'm still not sure it's more readable to cozy up multiline argument lists into a single line: [...]
My main gripe with Black is how it forcibly unwraps method parameters (both for method calls and definitions), method chaining, comprehensions (list, sets, generators), ternary conditions, etc. when I put them on multiple lines for readability reasons to begin with. Even worse when some lines are right at the limit, and some aren't.
Hence why I'm still using autopep8, even if I dislike the 2-blank-lines spacing.
[...] I think the long term viability of both blue and black is questionable, given the existence of ruff. I'm hoping that once ruff implements autoformatting **and gives us a choice [...]
I too am really looking for it. Been loving Ruff. Hopefully it's configurable enough. Relevant discussions on Ruff's side: https://github.com/astral-sh/ruff/issues/1904 Progress: https://github.com/astral-sh/ruff/issues/4798
The single major advantage of Black is that it removes the necessity of having to discuss something as trivial as style.
First of all, style is not trivial, because it greatly affects readability and thus people's productivity.
For example, this (Black
's default) is just hard to read:
def myFunc(
myFuncArg1,
myFuncArg2,
myFuncArg3,
):
pass
And does the existence of Black
eliminate discussion of code style? No, it doesn't. On the contrary, it resulted in more heated discussions on GitHub, on Reddit, etc.
style is not trivial
True
it resulted in more heated discussions
Not untrue. 😄
Maybe "trivial" isn't the right word for it, but most developers say in a corporate environment, really really don't want to argue about it. Every project has its coding style, either developed by Python experts in a corporation, or project owners in an OSS project. Contributors and engineers just want to be told what that style is, and they want a tool that will just DTRT. So maybe not "trivial" but "don't make me think".
I know that as a project owner and Python leader who reviews a ton of PRs, I absolutely do not want to review style violations. I don't want to fix them and I don't want to demoralize contributors telling them to move a parenthesis or change their quoting style. I want my test infrastructure and CI to gate on style violations so that all of that's cleaned up before I get to reviewing the code. That way, the machines can do the dirty work and I can concentrate on reviewing the actual substance of the PR.
No 'sadface dedent'!
I would like a setting for "Trailing bracket indent" or similar. Make it a number of spaces in from the first character of the line that starts the multiline construct. (0 is black's opinion, mine is different, probably 8 for most cases) As to why...
I agree that the following (from above comment) is horrible.
def myFunc(
myFuncArg1,
myFuncArg2,
myFuncArg3,
):
pass
The rationale for black doing it this way is threefold.
My counter arguments in increasing order of length: No 3 is very useful, but can be achieved with any indent level. As long as the closing bracket is on its own line. I don't think it's a strong argument. No 1, I think they are all being done wrong. In python, dedenting means you are into another construct. We should be finishing all our bracketed constructs with a bracket at the same level as the content of the construct. This is PEP8 compliant. eg: Dictionary literals
directives = {
'function': EQLFunctionDirective,
'constraint': EQLConstraintDirective,
'type': EQLTypeDirective,
'keyword': EQLKeywordDirective,
'operator': EQLOperatorDirective,
'synopsis': EQLSynopsisDirective,
'react-element': EQLReactElement,
'section-intro-page': EQLSectionIntroPage,
'struct': EQLStructElement,
}
Calling functions
if ret is DEFAULT:
ret = self._get_child_mock(
_new_parent=self, _new_name='()'
)
self.return_value = ret
Importing many names
from typing import (
Iterator,
List,
Sequence,
Set,
)
Complex boolean expressions
create_flag = (
not self.disable_multitouch
and not self.multitouch_on_demand
)
That way, the next construct is at a lesser indentation, as you would expect in Python. I think this is a strong counter argument to the 'sadface dedent'.
That brings us to No 2, blocks. I think that a ): on a line on its own is a reasonable delimiter, but I concede that others may prefer a more obvious break. PEP8 is explicitly silent on this case for an 'if' block, but a similar situation in function/method definitions is disallowed. Given we want consistency, the do nothing option (only allowed for if) is out. Another option is a comment. I don't think a code formatter should add comments. The third option in PEP8 is further indentation on the arguments:
if (
not line
or (line[0] == "#")
or (line[:3] == '"""')
or line[:3] == "'''"
):
self.error("Blank or comment")
return
This looks a little empty, but a def is more normal looking:
def myFunc(
myFuncArg1,
myFuncArg2,
myFuncArg3,
):
pass
Despite black's assertion that custom indents Smaller than 4 spaces would violate PEP 8 PEP8 also says that The 4-space rule is optional for continuation lines. So maybe if conditions and function parameters should be at 2 spaces in? I think this is quite readable.
if (
not line
or (line[0] == "#")
or (line[:3] == '"""')
or line[:3] == "'''"
):
self.error("Blank or comment")
return
for a def
def myFunc(
myFuncArg1,
myFuncArg2,
myFuncArg3,
):
pass
or, if that isn't distinct enough and you want more indentation
if (
not line
or (line[0] == "#")
or (line[:3] == '"""')
or line[:3] == "'''"
):
self.error("Blank or comment")
return
for a def
def myFunc(
myFuncArg1,
myFuncArg2,
myFuncArg3,
):
pass
Still quite readable. If you want a really clear distinction, go with 8 spaces (double indent).
The author of black says that there won't be enough space with a double indent and that if conditions and function parameters will be overly constrained, especially when defining functions with type hints. While an if could be indented at some levels in from the left before you get into conditions, how complexly nested is your code? Maybe it needs a refactor if this is a problem. For function parameters and class methods, if you used 6-8 spaces for the extra indent, you should have 80-82 for your parameter names with type hints (4 less if defining a method in a class). Is that really not enough? What are you doing? Finally, the conclusion given a the end of the justification is:
The "sadface dedent" style is objectively better even if it’s subjectively uglier.
Ridiculous! The matter at hand is "code style", an entirely subjective thing. We're talking about styling, functionality is unchanged. The only way in which a 0 indent is objectively better than some other value with equal distinction (say 8), is when the bigger indent would cause the code to be mangled. This would be triggered by very long conditions on deeply indented ifs, or on parameters (and type hints) that were close to 80 characters long. I haven't written much (any?) code that would trigger these problems in my career. So let's not spoil all the blocks to save the ones I never use.
FWIW, the -C
command-line option of Black avoids situations like this:
from typing import (
Iterator,
List,
Sequence,
Set,
)
Use this to gather ideas for formatting improvements here.