black icon indicating copy to clipboard operation
black copied to clipboard

Allow newlines before parameter comments

Open Bengt opened this issue 2 years ago • 9 comments

Suppose we have a function with both named and unnamed parameters:

def print_characters(
    foo,
    /,
    *,
    bar,
    foobar,
):
    print(
        *foo,
        sep=foobar,
        end=bar,
    )

Currently, Black does not allow spaces before comments to parameters. Black formats like so:

print_characters(
    # Positional parameters
    "42",  # foo
    # Name-only parameters
    bar=".",
    foobar=", ",
)

I would like to use comments to structure the parameter sections like so:

print_characters(
    # Positional parameters
    '42',  # foo

    # Name-only parameters
    bar='.',
    foobar=', '
)

Note that the newlines before the section comments in the call to print_characters separate the positional-only from the name-only parameters in a way that reflects how /,\n*, separates them in the function definition.

Alternatives

One could use an empty comment for separation, but that does not seem too elegant:

print_characters(
    # Positional parameters
    '42',  # foo
    #
    # Name-only parameters
    bar='.',
    foobar=', '
)

Context

Please consider the playground for my example.

Bengt avatar Jan 24 '22 15:01 Bengt

Thanks for the submission! But I don't think this is good in general, meaning in all cases with a comment line between arguments. Also, I think this could be extended to cases without any comments.

Taking into account previous formatting (leaving user-inserted newlines) is not something we're too happy about. We do make an exception with vertical whitespace in other places, up to 1 empty line. So this could be chucked onto that bin, regardless of comments. But we've rejected similar requests for lists as well. It isn't really a good look:

[
    a,
    b,

    c,
]

So it's a tentative no from me, but maybe there's something to this.

felix-hilden avatar Jan 24 '22 15:01 felix-hilden

Thanks for the quick reply and your explanations! I get why you want to ignore inputs like this, which are not semantic to Python, but semantic to the reader. I think readability can benefit from newlines, even in the simpler list case. Say, we have a list of different types and want to make clear that we did that on purpose. A newline can make that clear, concisely:

[
    "a",
    "b",

    0,
    1,
]

Bengt avatar Jan 24 '22 15:01 Bengt

That's fair, but in that case a comment can help. Black won't analyse the list and determine that "A-ha, the list of strings ends there and afterwards there are only integers so let's put a space there".

The previous issue which also has this discussion is #2723. Paraphrasing from there, it isn't clear when the empty line is intentional or accidental. I think we'll close this issue with similar rationale. Comments are a great way of making the separation explicit.

Do let us know if there's a reasonable special case you're thinking of. But we want to aim for consistency. Hope you understand!

felix-hilden avatar Jan 24 '22 16:01 felix-hilden

I see. Thanks for linking and summarizing the older issue. I couldn't find it on my own. So, I guess, you would suggest using a comment for explicitly separating the list like so:

[
    # Strings
    "a",
    "b",
    # Integers
    0,
    1,
]

That comes very close to the way Black formats my parameters (as stated above):

print_characters(
    # Positional parameters
    "42",  # foo
    # Name-only parameters
    bar=".",
    foobar=", ",
)

I still take issue with that, as I think both variants would be more legible with newlines before the comments, like so:

[
    # Strings
    "a",
    "b",

    # Integers
    0,
    1,
]

and

print_characters(
    # Positional parameters
    "42",  # foo
    
    # Name-only parameters
    bar=".",
    foobar=", ",
)

Couldn't Black check if the newlines are followed by a comment line and not remove them in these cases? A single user-inserted newline before a comment-only line seems to be most likely intentional and for legibility reasons.

Bengt avatar Jan 24 '22 16:01 Bengt

Oh, a bit negligent of me. That is the special case. And I see your point: particularly before comments, the newlines can indeed improve readability.

Consider me slightly swayed, and thanks for pressing the matter! Thoughts by others?

felix-hilden avatar Jan 24 '22 16:01 felix-hilden

Cool. Thanks for reconsidering, @felix-hilden!

Bengt avatar Jan 24 '22 16:01 Bengt

I also find it useful in complex calls (or dictionaries/lists etc) to comment/document some of the parameters being passed, with whitespace before that comment for readability, and actively dislike black removing it for me.

Consider these two examples:

# doing this
x = blah()
x and zzz()

# then something else
if x:
    zap()

and

some_map = dict(
    # some relevant comments about these keys
    x=1,
    y=2

    # then comments about these which are different
    z=3,
)

In the first case, black does not see fit to remove the whitespace that was added for legibility. That's good. But in the latter case it does which makes the code less legible. I think a good general rule (no idea how easy it is to implement) is that if a comment is preceded by whitespace lines, then those whitespace should be compressed to one blank line and retained.

  • unless, I guess, its at the beginning of a stanza of some kind, in which case they should disappear ... so, perhaps not too easy

donkopotamus avatar Mar 01 '22 03:03 donkopotamus

Ha, "will close this with a similar rationale to #2723" Black didn't make it in our team and I advised others against using it

fmalina avatar Apr 23 '22 15:04 fmalina

Thanks for your input, @fmalina. Indeed, this issue is also blocking my projects to adopt black for now. I consider having strict rules a benefit, in regard to code style and quality, which is why I would like to use black. However, feel like black is overreaching when it reformats user code in cases where the correct way is not clear. More so, in cases like this one, where I think the better solution is actually the user input and not what black outputs.

Bengt avatar Apr 24 '22 19:04 Bengt

I have a similar issue to this where I am using blank lines before comments to separate out steps in a didactic example. For example this:


# Write the dataframe with sparklines and some additional formatting.
df.write_excel(
    workbook="polars_sparklines.xlsx",

    # Set an alternative table style.
    table_style="Table Style Light 2",

    # Specify an Excel number format for integer types.
    dtype_formats={INTEGER_DTYPES: "#,##0_);(#,##0)"},

    # Configure sparklines to the dataframe.
    sparklines={
        # We use the default options with just  the source columns.
        "Trend": ["Q1", "Q2", "Q3", "Q4"],

        # We also add a customized sparkline type, with a positioning directive.
        "Change": {
            "columns": ["Q1", "Q2", "Q3", "Q4"],
            "insert_after": "Zone",
            "type": "win_loss",
        },
    },
    column_totals=["Q1", "Q2", "Q3", "Q4"],

    # Hide the default gridlines on the worksheet.
    hide_gridlines=True,
)

Becomes (for me) less readable and less distinct using the current HEAD black:

# Write the dataframe with sparklines and some additional formatting.
df.write_excel(
    workbook="polars_sparklines.xlsx",
    # Set an alternative table style.
    table_style="Table Style Light 2",
    # Specify an Excel number format for integer types.
    dtype_formats={INTEGER_DTYPES: "#,##0_);(#,##0)"},
    # Configure sparklines to the dataframe.
    sparklines={
        # We use the default options with just  the source columns.
        "Trend": ["Q1", "Q2", "Q3", "Q4"],
        # We also add a customized sparkline type, with a positioning directive.
        "Change": {
            "columns": ["Q1", "Q2", "Q3", "Q4"],
            "insert_after": "Zone",
            "type": "win_loss",
        },
    },
    column_totals=["Q1", "Q2", "Q3", "Q4"],
    # Hide the default gridlines on the worksheet.
    hide_gridlines=True,
)

jmcnamara avatar Mar 11 '23 12:03 jmcnamara

@jmcnamara it does look better blacked IMO

fmalina avatar Apr 01 '23 11:04 fmalina