dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
650 stars 121 forks source link

[tall-style] Keep qualified names tighter than the containing expression #1557

Open rakudrama opened 2 months ago

rakudrama commented 2 months ago

Here the first js is a library import prefix. It should not IMO be separated from the thing it qualifies. (The formatting might be reasonable if the first js was a variable).

    return js.ExpressionStatement(
      js
          .js('# = #', [goto, js.number(label)])
          .withSourceInformation(sourceInformation),
    );

This would be better:

    return js.ExpressionStatement(
        js.js('# = #', [goto, js.number(label)])
        .withSourceInformation(sourceInformation),
    );

See https://dart-review.googlesource.com/c/sdk/+/383722/1/pkg/compiler/lib/src/js/rewrite_async.dart#315

munificent commented 2 months ago

I agree that the output here doesn't look great. That's often the case when you have a split call chain and the target (here just js) is shorter than 4 characters. It ends up looking like it's hanging out in space.

It's particularly egregious here because this isn't really a call chain with two method calls. It's a prefixed bare function call followed by a method call. Ideally, it would get formatted something like:

    return js.ExpressionStatement(
      js.js('# = #', [goto, js.number(label)])
          .withSourceInformation(sourceInformation),
    );

However, the formatter doesn't resolve anything. It doesn't know that js is a prefix and not an identifier expression.

You might argue that the formatter could know that because the prefixes are always easily textually found at the top of the file. However, that's not true when:

Sometimes I wish we used a different separator for prefixes, like ::. But the formatter has to deal with what it's given and format with very little context.

There is one hacky heuristic the formatter uses to guess at what a name means. It uses capitalization to guess that an identifier is a type name versus a function name. That shows up in things like named constructors and static methods. If you have Iterable.generate(), the formatter won't split on that . because it sees that the first identifier is capitalized. Likewise, if you have name.SomeClass(), it assumes name is a prefix and not an identifier expression because of the subsequent capitalized name. Therefore it won't split after name.

However, that doesn't help here because the thing you're calling with a prefix is a top level function whose name is lowercase. :( If it was js.Js, then the formatter would give you:

    return js.ExpressionStatement(
      js.Js('# = #', [
        goto,
        js.number(label),
      ]).withSourceInformation(sourceInformation),
    );

But of course, you don't want to give a function a capitalized name.

So, I agree the output doesn't look great here, but it's probably about the best we can do. Sorry I don't have a great solution. :(