Redundant newline with shorthand (=>)

[Empty2k12]

I guess this is the inverse from #721.

Actual Output:

  static HandlerResponse<RouteState> handleRouteReplaced(
    ReplaceRoute action,
    RouteState state,
  ) =>
      HandlerResponse.updateOnly(
        state.copy(active: action.route, history: state.history),
      );

Expected output:

  static HandlerResponse<RouteState> handleRouteReplaced(
    ReplaceRoute action,
    RouteState state,
  ) => HandlerResponse.updateOnly(
    state.copy(active: action.route, history: state.history),
  );

This does not happen when using regular braces instead of the shorthand, and only when the trailing comma has been added to collapse the overflowing method header (e.g. void hello() => doThat() formats correctly).

[Empty2k12]

Found an even more extreme example:

  @override
  Future<void> report(
    String tag,
    String message
  }) =>
      Future.value(null);

this should not wrap.

[munificent]

@Hixie, what output would you expect here? I don't have any intuition for how Flutter-style block formatting should interact with =>.

[Hixie]

I would expect the first to become:

  static HandlerResponse<RouteState> handleRouteReplaced(
    ReplaceRoute action,
    RouteState state,
  ) {
    return HandlerResponse.updateOnly(
      state.copy(active: action.route, history: state.history),
    );
  }

...and the second to become:

  @override
  Future<void> report({
    String tag,
    String message,
  }) => Future.value(null);

...or, if it fits:

  @override
  Future<void> report({ String tag, String message }) => Future.value(null);

[munificent]

The first example has a multi-line => body, though. How would you format that?

[Hixie]

by turning it into a block with a return statement.

[Empty2k12]

by turning it into a block with a return statement.

I think that would be incompatible with the prefer_expression_function_bodies lint: https://dart-lang.github.io/linter/lints/prefer_expression_function_bodies.html

[Hixie]

That's why our analysis_options.yaml file says:

    # - prefer_expression_function_bodies # conflicts with https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods

[munificent]

by turning it into a block with a return statement.

I guess maybe I'm not being clear enough. If you were an automated formatter that was only able to make whitespace changes to a given piece of code, what would you do with that code?

[Hixie]

If you were an automated formatter that was only able to make whitespace changes to a given piece of code, what would you do with that code?

I would reject the premise of the question.

[munificent]

So should I just close this bug then? I don't know how to take action on it.

[srawlins]

Do issues of distaste with =>-followed-by-long-line-wrapped-statement come up frequently enough for its own "Why did the formatter..." section to the FAQ? Basically directing the user to perhaps use { return or otherwise extract complexity from the function expression body?

[munificent]

There are definitely many many multi-line => member bodies. As far as I can tell, most users are happy with the output, but most of the code I see does not use trailing commas in the parameter list, so those are formatted differently.

This issue is about combining:

• Trailing commas in the parameter list.
• => for the member body.
• A member body that doesn't fit on one line.

[Hixie]

So should I just close this bug then?

I don't know, this wasn't a bug I filed.

I did file #452 back in 2015, which was the bug requesting that we reject the premise that the formatter be artificially limited to only changing whitespace and instead also change things like switching between => ... and { return ...; } based on the length of the expression, so as to make developers able to apply the Flutter style guide's recommendation cited above without having to manually babysit the formatter, but that bug was closed without being fixed.

[munificent]

OK. Having dartfmt automatically switch between => and { return ... } at this point would be a very large, disruptive change. It may also be a change that the majority of users don't actually want. (Presumably, at least some of the time, if they chose to write =>, it probably means they want => and not { return ... }.

All that means I can't make that change unilaterally or any time soon, if ever.

If you don't have any suggestions for how to format the code in this bug better while only changing the whitespace then I guess I'll just leave it open and if I get time maybe I'll try to do something.

[Hixie]

OK. Having dartfmt automatically switch between => and { return ... } at this point would be a very large, disruptive change.

An impactful change, one might say.

It may also be a change that the majority of users don't actually want. (Presumably, at least some of the time, if they chose to write =>, it probably means they want => and not { return ... }.

I have no data to say whether people would want this one way or the other.

The argument that "if they chose to write =>, it probably means they want =>" contradicts the entire existence of dartfmt, though. After all, it is exactly the same argument as "if they chose to put a newline there, it probably means they want a newline there".

If you don't have any suggestions for how to format the code in this bug better while only changing the whitespace then I guess I'll just leave it open and if I get time maybe I'll try to do something.

SGTM. I don't have any suggestions for how to format this code under those constraints; doing so would violate our style guide so we try to avoid doing it at all.

[munificent]

An impactful change, one might say.

If the magnitude is large, all the more important to make sure the sign bit is actually positive.

[Empty2k12]

I don't have any suggestions for how to format this code under those constraints; doing so would violate our style guide so we try to avoid doing it at all.

If this code snippet is not formattable into a nice looking code via the Style guidelines, they are the ones needing a change. You can't build a car that can't go up hills and then blame it on the hill.

[Hixie]

If this code snippet is not formattable into a nice looking code

It is. The Flutter style guide says the way you format that is you change it to a block with a return.

[munificent]

The forthcoming tall style produces the desired output in both of these examples now:

class C {
  static HandlerResponse<RouteState> handleRouteReplaced(
    ReplaceRoute action,
    RouteState state,
  ) => HandlerResponse.updateOnly(
    state.copy(active: action.route, history: state.history),
  );
}

And:

class C {
  @override
  Future<void> report({
    String tag,
    String messageVeryLongParameterToForceSplit,
  }) => Future.value(null);
}