Tall style: Missing trailling comma when passing a closure with a block body to a function

[rrousselGit]

Hello again! I encountered the following when playing with the new style:

  Result<Res> chain<Res>(Res Function(T value) cb) {
    return when(
      data: (value) {
        try {
          return Result.data(cb(value));
        } catch (err, stack) {
          return Result.error(err, stack);
        }
      },
      error: Result.error,
    );
  }

  T get dataOrThrow {
    return when(
      data: (value) => value,
      error: (err, stack) {
        // ignore: only_throw_errors
        throw err;
      },
    );
  }

Became:

  Result<Res> chain<Res>(Res Function(T value) cb) {
    return when(data: (value) {
      try {
        return Result.data(cb(value));
      } catch (err, stack) {
        return Result.error(err, stack);
      }
    }, error: Result.error);
  }

  T get dataOrThrow {
    return when(data: (value) => value, error: (err, stack) {
      // ignore: only_throw_errors
      throw err;
    });
  }

I don't mind so much that the formatter removes the trailing comma when a "block" is the last parameter of function invocation. So I'm not against the dataOrThrow changes.

But I think that for chain, that's a big downgrade.

It is very difficult to stop that error: Result.error named parameter with the new change. It's easy for the eye to gloss over that parameter, by thinking that it's part of the logic of the block above.

IMO, if a closure with a block body is used and isn't the last function parameter, we should have a trailing comma. This would make it easier to view all named parameters.

[munificent]

Yeah, as I mentioned on #1528, getting the heuristics right for when arguments should get special block-like formatting versus forcing the entire argument list to split is a subtle art. I don't hate the output in your example here, but I also see what you're saying and don't particularly love it either.

Part of the motivation for the current rules is to make sure that we keep block-formatting the function expressions passed to test() and group(). Those functions do take other named parameters. Even if a user passes, say, tags to test(), they really don't want the whole test closure to end up indented like:

test(
  'Test stuff',
  () {
    // Oof, too much indentation...
    expect(stuff, otherStuff);
  },
  tags: ['hi'],
);

We might tweak things by saying that if a closure argument is named and there are other named arguments, then you don't block format the closure. That would address your example without making test() and group() calls look worse.

[rrousselGit]

test(
  'Test stuff',
  () {
    // Oof, too much indentation...
    expect(stuff, otherStuff);
  },
  tags: ['hi'],
);

IMO that's perfectly correct. The indentation is important to vidiaully differentiate the logic from parameters here.

But even better:

test('Test stuff', tags: ['hi'], () {

});

Named arguments everywhere solve those cases.

[rrousselGit]

I wouldn't differentiate between named or not. Only on a single information: Is it the last parameter or not.

Folks can then move the parameter around to match their needs.

[munificent]

Yeah, the heuristic definitely needs some tweaks. Here's some examples from Flutter:

    // Before:
    return Scaffold(
      body: Center(
        child: AnimatedDigit(value: value % 10),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          setState(() { value += 1; });
        },
        tooltip: 'Increment Digit',
        child: const Icon(Icons.add),
      ),
    );

    // After:
    return Scaffold(
      body: Center(child: AnimatedDigit(value: value % 10)),
      floatingActionButton: FloatingActionButton(onPressed: () {
        setState(() {
          value += 1;
        });
      }, tooltip: 'Increment Digit', child: const Icon(Icons.add)),
    );

And:

// Before:
final CatmullRomSpline path = CatmullRomSpline(
  const <Offset>[
    Offset(0.05, 0.75),
    Offset(0.18, 0.23),
    Offset(0.32, 0.04),
    Offset(0.73, 0.5),
    Offset(0.42, 0.74),
    Offset(0.73, 0.01),
    Offset(0.93, 0.93),
    Offset(0.05, 0.75),
  ],
  startHandle: const Offset(0.93, 0.93),
  endHandle: const Offset(0.18, 0.23),
);

// After:
final CatmullRomSpline path = CatmullRomSpline(const <Offset>[
  Offset(0.05, 0.75),
  Offset(0.18, 0.23),
  Offset(0.32, 0.04),
  Offset(0.73, 0.5),
  Offset(0.42, 0.74),
  Offset(0.73, 0.01),
  Offset(0.93, 0.93),
  Offset(0.05, 0.75),
], startHandle: const Offset(0.93, 0.93), endHandle: const Offset(0.18, 0.23));