Git Product home page Git Product logo

Comments (25)

warsaw avatar warsaw commented on July 18, 2024 32

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

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

from blue.

merwok avatar merwok commented on July 18, 2024 20

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!

from blue.

warsaw avatar warsaw commented on July 18, 2024 19

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

from blue.

warsaw avatar warsaw commented on July 18, 2024 17

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.

from blue.

stefanoborini avatar stefanoborini commented on July 18, 2024 13

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)

from blue.

warsaw avatar warsaw commented on July 18, 2024 12

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

from blue.

warsaw avatar warsaw commented on July 18, 2024 9

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

from blue.

warsaw avatar warsaw commented on July 18, 2024 9

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

from blue.

spaceone avatar spaceone commented on July 18, 2024 7

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.

from blue.

warsaw avatar warsaw commented on July 18, 2024 6

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.

from blue.

stefanoborini avatar stefanoborini commented on July 18, 2024 5

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

from blue.

warsaw avatar warsaw commented on July 18, 2024 5

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.

from blue.

merwok avatar merwok commented on July 18, 2024 2

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.

from blue.

stefanoborini avatar stefanoborini commented on July 18, 2024 2

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

from blue.

jalanb avatar jalanb commented on July 18, 2024 2

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

from blue.

jsh9 avatar jsh9 commented on July 18, 2024 2

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.

from blue.

allaboutmikey avatar allaboutmikey commented on July 18, 2024 2

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.

from blue.

AXGKl avatar AXGKl commented on July 18, 2024 1

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

from blue.

stefanoborini avatar stefanoborini commented on July 18, 2024 1

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.

from blue.

stefanoborini avatar stefanoborini commented on July 18, 2024 1

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.

from blue.

grantjenks avatar grantjenks commented on July 18, 2024

Moved ideas to individual tickets ...

from blue.

boxed avatar boxed commented on July 18, 2024

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.

from blue.

merwok avatar merwok commented on July 18, 2024

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

from blue.

Avasam avatar Avasam commented on July 18, 2024

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

from blue.

tylerlaprade avatar tylerlaprade commented on July 18, 2024

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

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

from blue.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.