Unsplit list literal in method chain formats poorly

[escamoteur]

Whe using fluent function calls the results of dartfmt isn't optimal Especially when using RxDart this produces real ugly results like:

    filterUpdateSubscription = sl
        .get<EventManager>()
        .updateFilterCommand
        .results
        .mergeWith([sl.get<PlaceManager>().updateFilterCommand.results]).listen(
            (filterState) async {
      AppKeys.mapView?.currentState?.activateMapOverlay();
      await Future.delayed(Duration(milliseconds: 100));
      setState(() {
        filterIsActive = filterState;
      });
    });

Where this

    filterUpdateSubscription = sl.get<EventManager>().updateFilterCommand
        .results
          .mergeWith([sl.get<PlaceManager>().updateFilterCommand.results])
            .listen( (filterState) async {
                AppKeys.mapView?.currentState?.activateMapOverlay();
                await Future.delayed(Duration(milliseconds: 100));
                setState(() {
                  filterIsActive = filterState;
                });
              });

Would be much easier to read.

I know that dartfmt has the goal of making all dart code look the same but if this works against readability of the intent it's not a good philosophy.

I would propose a simple option to be able to tell dartfmt to not touch lines beginning with a '.'

[sir-boformer]

I actually prefer the first format where every . line starts with the same indentation.

[escamoteur]

I could live with the same indentation but look where it put the .listen() also directly making a line-break after the sl is not what is intended the first method call returns the value on that is the start of the execution pipeline.

[srawlins]

I do find it puzzling to see that .listen( not on its own line... I think there is a dartfmt "bug" here with the list literal, @munificent. If we pull the list literal out into a variable (even a crazy one whos name is the same length as the literal), the formatting gets all better:

var placeManagerUpdateResults___________________________ = [
  sl.get<PlaceManager>().updateFilterCommand.results
];

var filterUpdateSubscription = sl
    .get<EventManager>()
    .updateFilterCommand
    .results
    .mergeWith(placeManagerUpdateResults___________________________)
    .listen((filterState) async {
  AppKeys.mapView?.currentState?.activateMapOverlay();
  await Future.delayed(Duration(milliseconds: 100));
  setState(() {
    filterIsActive = filterState;
  });
});

[escamoteur]

@srawlins That would make it much better although I wished the handler function block of the listen would be indented more than the .listen and chance to achieve that?

[escamoteur]

Could it be a solution to give dartfmt a hint where to break a line by inserting a space before a '.' ? Like we do with trailing ','

[munificent]

It is a non-goal to let users opt out of formatting.

I agree the output in this particular case is not great. :( Like @srawlins notes, it has to do with the list literal. dartfmt has some surprisingly complex rules to handle function and collection literals inside arguments in order to more gracefully handle cases like:

function([
  element,
  element,
  element,
  element,
  element,
  element,
]).method();

Most users prefer that over:

function([
      element,
      element,
      element,
      element,
      element,
      element,
    ])
    .method();

Unfortunately, due to some limitations in the line splitting algorithm, it can't currently gracefully switch between allowing a trailing method call on the same line when the list literal does split while preventing it when it doesn't.

So in your example, it sees the list literal, and prepares to format it like:

    filterUpdateSubscription =
        sl.get<EventManager>().updateFilterCommand.results.mergeWith([
      sl.get<PlaceManager>().updateFilterCommand.results
    ]).listen((filterState) async {
      AppKeys.mapView?.currentState?.activateMapOverlay();
      await Future.delayed(Duration(milliseconds: 100));
      setState(() {
        filterIsActive = filterState;
      });
    });

But, then, because the list literal does all fit in one line, it switches things around, but still leaves the .listen() on the same line.

It's something I'd really like to improve, but I haven't been able to yet.

[escamoteur]

What about the idea to give dartfmt a hint like we can do by adding a trailing comma? So that by adding a space dartfmt knows where to break the line?

Another question: Why does dartfmt not indent the block of anonymous functions so that its end brace is not on the same depth as the rest of the surrounding code?

[munificent]

What about the idea to give dartfmt a hint like we can do by adding a trailing comma? So that by adding a space dartfmt knows where to break the line?

I consider the trailing comma "hint" to be a major problem in dartfmt. I increasingly see code that inconsistently applies trailing commas and ends up with wildly inconsistent formatting in a single file.

Why does dartfmt not indent the block of anonymous functions so that its end brace is not on the same depth as the rest of the surrounding code?

It looks better to format it like a block in a lot of common cases. Consider:

  group('textBeforeSelection', () {
    test('gets substring before selection', () {
      expect(selection.textBeforeSelection, equals("123"));
    });

    test('gets entire string if no selection', () {
      expect(noSelection.textBeforeSelection, equals("123456;"));
    });
  });

Versus:

  group(
      'textBeforeSelection',
      () {
        test(
            'gets substring before selection',
            () {
              expect(selection.textBeforeSelection, equals("123"));
            });

        test(
            'gets entire string if no selection',
            () {
              expect(noSelection.textBeforeSelection, equals("123456;"));
            });
      });

[escamoteur]

No, that would be ugly I meant just something like this

group('textBeforeSelection', () {
  test('gets substring before selection', () {
      expect(selection.textBeforeSelection, equals("123"));
    });

  someOtherMethodCall();

  test('gets entire string if no selection', () {
      expect(noSelection.textBeforeSelection, equals("123456;"));
    });
});

[munificent]

Why are the bodies of test() indented but group() is not? Why is this better than how it currently formats it?

[escamoteur]

Sorry, the example was wrong. I was more thinginh about the block of anonymous function blocks like

    return Observable(_databaseService.getChatEntries(
      event,
    )).map((entryList) {
      entryList.forEach(
          (entry) => entry.isFromCurrentUser = (entry.userId == _userManager.currentUser.id));
      return entryList;
    });

vs.

    return Observable(_databaseService.getChatEntries(
      event,
    )).map((entryList) {
        entryList.forEach(
            (entry) => entry.isFromCurrentUser = (entry.userId == _userManager.currentUser.id));
        return entryList;
      });

[zoechi]

The 2nd variant would make me very nervous.
It is a complete statement that ends with closing paren but not with the indentation of the beginning. It's completely unclear what it closes.

Adding , between }); like `},); might be closer to what you want.

  return Observable(_databaseService.getChatEntries(
    event,
  )).map(
    (entryList) {
      entryList.forEach((entry) => entry.isFromCurrentUser =
          (entry.userId == _userManager.currentUser.id));
      return entryList;
    },
  );

[munificent]

It's pretty common to chain higher-order functions. In your example, would they march farther and farther to the right?

    return Observable(_databaseService.getChatEntries(
      event,
    )).map((entryList) {
        entryList.forEach(
            (entry) => entry.isFromCurrentUser = (entry.userId == _userManager.currentUser.id));
        return entryList;
      }).where((entry) {
          return entry != null;
        }).map((entry) {
            return entry.toString();
          });

I don't think many users would be happy with that.

[escamoteur]

Which is quite usual in other programming languages. Especially with the block brackets it is way better readable

[munificent]

Earlier, I said:

Unfortunately, due to some limitations in the line splitting algorithm, it can't currently gracefully switch between allowing a trailing method call on the same line when the list literal does split while preventing it when it doesn't.

One of my main goals with designing the new intermediate represention and line splitter was to fix this exact limitation. With the forthcoming tall style, you get:

    filterUpdateSubscription = sl
        .get<EventManager>()
        .updateFilterCommand
        .results
        .mergeWith([sl.get<PlaceManager>().updateFilterCommand.results])
        .listen((filterState) async {
          AppKeys.mapView?.currentState?.activateMapOverlay();
          await Future.delayed(Duration(milliseconds: 100));
          setState(() {
            filterIsActive = filterState;
          });
        });

That looks pretty good to me.