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

Accumulate formatting ideas here #2

Closed
grantjenks opened this issue Jan 8, 2021 · 25 comments
Closed

Accumulate formatting ideas here #2

grantjenks opened this issue Jan 8, 2021 · 25 comments

Comments

@grantjenks
Copy link
Owner

Use this to gather ideas for formatting improvements here.

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

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.

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

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('.'))

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

blue should not de-parenthesize ternary operators. The parentheses aid in readability.

-SEP = ('^' if sys.platform == 'win32' else '|')
+SEP = '^' if sys.platform == 'win32' else '|'

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

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
+    ):

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

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)),
+            )
+        )

@warsaw
Copy link
Collaborator

warsaw commented Jan 12, 2021

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.]*'

@grantjenks
Copy link
Owner Author

Moved ideas to individual tickets ...

@merwok
Copy link

merwok commented Jan 25, 2021

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!

@axgkl
Copy link

axgkl commented Jul 20, 2022

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:

  1. Maybe introduce a CLI flag which allows to triple single quote non docstrings

  2. and 3. Check before breaking if the line would fit into a single line and if so: Not break line.

  3. and 3. I have in axblack, from which I forward now to this project (hope that's ok).

@stefanoborini
Copy link

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)

@merwok
Copy link

merwok commented Dec 22, 2022

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.

@stefanoborini
Copy link

@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.

@spaceone
Copy link

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.

@stefanoborini
Copy link

@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.

@boxed
Copy link

boxed commented Apr 19, 2023

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.

@jalanb
Copy link

jalanb commented Jun 1, 2023

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".

@stefanoborini
Copy link

The single major advantage of Black is that it removes the necessity of having to discuss something as trivial as style.

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.

@merwok
Copy link

merwok commented Jun 6, 2023

Not sure if you’re saying blue or black should not exist, and whether you fully support black or blue.

@stefanoborini
Copy link

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.

@warsaw
Copy link
Collaborator

warsaw commented Jun 6, 2023

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.

@Avasam
Copy link

Avasam commented Jun 23, 2023

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: astral-sh/ruff#1904
Progress: astral-sh/ruff#4798

@jsh9
Copy link

jsh9 commented Jun 23, 2023

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.

@warsaw
Copy link
Collaborator

warsaw commented Jun 23, 2023

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.

@allaboutmikey
Copy link

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.

  1. All Bracket Pairs are treated equal.
  2. Clearly delimited block header.
  3. Minimize diffs.

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.

@tylerlaprade
Copy link

tylerlaprade commented Jul 19, 2023

FWIW, the -C command-line option of Black avoids situations like this:

from typing import (
    Iterator,
    List,
    Sequence,
    Set,
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests