Poor formatting on iterable pipeline

[kentcb]

Here is some real code formatted by the Dart formatter:

      final listViewChildren = <ChronologicalGroup>[
        ChronologicalGroup.today,
        ChronologicalGroup.thisWeek,
        ChronologicalGroup.thisMonth,
        ChronologicalGroup.lastMonth,
        ChronologicalGroup.later,
      ]
          .map((chronologicalGroup) => viewModel.jobs[chronologicalGroup])
          .where((jobsInChronologicalGroup) => jobsInChronologicalGroup != null)
          .expand((jobsInChronologicalGroup) {
        final header = ListTile(
          title: Text(
            _headerForChronologicalGroup(jobsInChronologicalGroup.first.chronologicalGroup),
            style: Theme.of(context).textTheme.title,
          ),
        );

        return <Widget>[header].followedBy(jobsInChronologicalGroup.map((job) => _JobView(job)));
      }).toList();

The indentation is off. I would expect something like (at worst):

      final listViewChildren = <ChronologicalGroup>[
        ChronologicalGroup.today,
        ChronologicalGroup.thisWeek,
        ChronologicalGroup.thisMonth,
        ChronologicalGroup.lastMonth,
        ChronologicalGroup.later,
      ]
          .map((chronologicalGroup) => viewModel.jobs[chronologicalGroup])
          .where((jobsInChronologicalGroup) => jobsInChronologicalGroup != null)
          .expand((jobsInChronologicalGroup) {
              final header = ListTile(
                title: Text(
                  _headerForChronologicalGroup(jobsInChronologicalGroup.first.chronologicalGroup),
                  style: Theme.of(context).textTheme.title,
                ),
              );

              return <Widget>[header].followedBy(jobsInChronologicalGroup.map((job) => _JobView(job)));
            })
          .toList();

[munificent]

What would you expect at best?

[kentcb]

Didn't really think about it too hard, but I thought that maybe the array could line up better with the operators in the pipeline. Something like:

final listViewChildren = <ChronologicalGroup>[
        ChronologicalGroup.today,
        ChronologicalGroup.thisWeek,
        ChronologicalGroup.thisMonth,
        ChronologicalGroup.lastMonth,
        ChronologicalGroup.later,
      ]
      .map((chronologicalGroup) => viewModel.jobs[chronologicalGroup])
      .where((jobsInChronologicalGroup) => jobsInChronologicalGroup != null)
      .expand((jobsInChronologicalGroup) {
          final header = ListTile(
            title: Text(
              _headerForChronologicalGroup(jobsInChronologicalGroup.first.chronologicalGroup),
              style: Theme.of(context).textTheme.title,
            ),
          );

          return <Widget>[header].followedBy(jobsInChronologicalGroup.map((job) => _JobView(job)));
        })
      .toList();

[hh10k]

I'm just starting out in Dart, coming from JavaScript, and I find this behaviour very strange too. I would expect that the closing } of a lamda would be indented to at least the level of the line that the starting { was on. In this case, the } before toList would greater or equal to the level of .expand.

You can improve the situation by adding a trailing comma but then the lambda will still be indented one less that the function that it's used in:

Stream<dynamic> refreshEpic(
    Stream<dynamic> actions, EpicStore<AppState> store) {
  return Observable(actions)
      .ofType(TypeToken<RefreshAction>())
      .switchMap(
    (action) {
      return Observable.just(RefreshSuccessAction())
          .delay(Duration(seconds: 2));
    },
  );
}

It's not easy to understand quickly.

[munificent]

I would expect that the closing } of a lamda would be indented to at least the level of the line that the starting { was on. In this case, the } before toList would greater or equal to the level of .expand.

It does that in some casts, but in other places, it tries to format the function more like a "block". For example, here:

import 'package:test/test.dart' as t;

main() {
  t.test(() {
    // Some big chunk of code.
  });
}

It would be bad if it turned that into:

import 'package:test/test.dart' as t;

main() {
  t.test(
      () {
        // Some big chunk of code.
      });
}

The heuristics it has for when to use block-like formatting and when not too are unfortunately fairly subtle and complex. They do a good job most of the time, but sometimes they end up picking wrong.

The trailing comma example you note is even worse. That's because trailing commas were added late to the language and they interact with just about every formatting rule in sometimes complex ways and we haven't finished flushing out all of the issues caused by those interactions. :(

[munificent]

The forthcoming tall style gives you:

      final listViewChildren =
          <ChronologicalGroup>[
                ChronologicalGroup.today,
                ChronologicalGroup.thisWeek,
                ChronologicalGroup.thisMonth,
                ChronologicalGroup.lastMonth,
                ChronologicalGroup.later,
              ]
              .map((chronologicalGroup) => viewModel.jobs[chronologicalGroup])
              .where(
                (jobsInChronologicalGroup) => jobsInChronologicalGroup != null,
              )
              .expand((jobsInChronologicalGroup) {
                final header = ListTile(
                  title: Text(
                    _headerForChronologicalGroup(
                      jobsInChronologicalGroup.first.chronologicalGroup,
                    ),
                    style: Theme.of(context).textTheme.title,
                  ),
                );

                return <Widget>[header].followedBy(
                  jobsInChronologicalGroup.map((job) => _JobView(job)),
                );
              })
              .toList();

And:

Stream<dynamic> refreshEpic(
  Stream<dynamic> actions,
  EpicStore<AppState> store,
) {
  return Observable(actions).ofType(TypeToken<RefreshAction>()).switchMap((
    action,
  ) {
    return Observable.just(RefreshSuccessAction()).delay(Duration(seconds: 2));
  });
}

I think those look fairly close to the desired output and is about as good as I'd hope.