Assert statement condition broken up in unfortunate way

[madig]

Operating system: Win 10 1803 x64
Python version: 3.6.5
Black version: 18.6b2
Does also happen on master: yes

Black outputs the following code in one project (googlefonts/fontmake@e22a624):

        assert len(ufo_order) == len(
            ot_order
        ), "{}, subsetting: amount of glyphs does not match with {}".format(
            ufo.path, otf_path
        )

The placement of the assert condition is unfortunate... Is this expected? Is there a more elegant way to write that code?

[ambv]

Black prefers to split using existing bracket pairs compared to putting extra parentheses itself. This usually looks better but in your case I agree it would look a bit better formatted as:

...
        assert (
            len(ufo_order) == len(ot_order)
        ), "{}, subsetting: amount of glyphs does not match with {}".format(
            ufo.path, otf_path
        )

I'll look into what I can do to improve this. Changes to this do end up touching unexpected formattings elsewhere so I'm trying to be conservative about this.

[madig]

Found another unfortunate break-up:

    tabular_kerning_groups = set()
    for pair in ufo_font.kerning.keys():
        if any(group in pair for group in tabular_groups) or any(
            figure in pair for figure in tabular_figures
        ):
            tabular_kerning_groups.add(pair)

Might be better as:

    tabular_kerning_groups = set()
    for pair in ufo_font.kerning.keys():
        if (
            any(group in pair for group in tabular_groups)
            or any(figure in pair for figure in tabular_figures)
        ):
            tabular_kerning_groups.add(pair)

[ambv]

Your second example is a duplicate of #260. In essence, Black doesn't understand symmetry and to make it understand symmetry it will slow things down significantly so I have to be smart about it. We'll address it some day but there are clearer bugs that I'll have to get to first.

[felix-hilden]

Here's a playground for convenience. It formats it differently this time around:

assert len(ufo_order) == len(
    ot_order
), "{}, subsetting: amount of glyphs does not match with {}".format(ufo.path, otf_path)

Although personally I'd like to have it like this:

assert (
    len(ufo_order) == len(ot_order),
    "{}, subsetting: amount of glyphs does not match with {}".format(ufo.path, otf_path)
)

which is just barely below the line length limit, but reformatted to:

assert (
    len(ufo_order) == len(ot_order),
    "{}, subsetting: amount of glyphs does not match with {}".format(
        ufo.path, otf_path
    ),
)

because of a trailing comma that would be added that would get it over the limit 😄 which is ok too in my opinion.

[felix-hilden]

The second example is still being discussed in #2156

[felix-hilden]

Woah let's pull some brakes with the style I proposed: assert considers a tuple argument to only specify the first element, so wrapping parens won't work here. So to achieve something close to what I proposed we'd have to use two sets of outer parens:

assert (
    # condition explosion
), (
    # msg explosion
)

[dmwyatt]

assert considers a tuple argument to only specify the first element

FWIW, assert considers any non-empty tuple to be true (because that's how python's truthiness works). The first element doesn't matter. PEP-679 attempts to address this, but it is not yet accepted.

[felix-hilden]

Yeah, that was poorly worded, thanks for the clarification!