Unnecessary split between named argument and `=>` function expression

[chris-NR]

The new formatter has changed the location of added line breaks within a parameter list to immediately after the named parameter token. This has resulted in some significant readability issues compared to the previous formatter. Here are two examples:

The named parameter 'child' followed by a switch expression

Before

After

The named parameter 'children' followed by a list literal

Before

After

[charlesverge]

It appears as if I'm encoring this issue as well. Oddly enough, the following formatting only happens on the linux platform and not on mac os x.

diff --git a/lib/main.dart b/lib/main.dart
index 24b86a2..e5f9e8e 100644
--- a/lib/main.dart
+++ b/lib/main.dart
@@ -50,19 +50,18 @@ class SetupApp extends StatelessWidget {
   @override
   Widget build(BuildContext context) {
     return ScreenUtilInit(
-      builder: (_, __) => GetMaterialApp(
-          builder: (BuildContext context, Widget? child) {
-            final MediaQueryData data = MediaQuery.of(context);
-            return MediaQuery(
-              data: data.copyWith(textScaler: TextScaler.noScaling),
-              child: child!,
-            );
-          },
-          title: ConstantStringShared.appName,
-          debugShowCheckedModeBanner: false,
-          theme: kAppTheme,
-          initialRoute: RouteNames.splash,
-          getPages: AppPages.pages),
+      builder:
+          (_, __) => GetMaterialApp(
+            builder: (BuildContext context, Widget? child) {
+              final MediaQueryData data = MediaQuery.of(context);
+              return MediaQuery(data: data.copyWith(textScaler: TextScaler.noScaling), child: child!);
+            },
+            title: ConstantStringShared.appName,
+            debugShowCheckedModeBanner: false,
+            theme: kAppTheme,
+            initialRoute: RouteNames.splash,
+            getPages: AppPages.pages,
+          ),
       designSize: const Size(414, 894), // width and height of design file
     );
   }
Updated 1 path from the index```

[munificent]

Hi, @chris-NR, can you please include the code samples as text and not screenshots? Also, please include enough of the surrounding code so that it's a complete statement. Thanks.

[chris-NR]

Code for the first example

class StatusIndicator extends StatelessWidget {
  final MachineStatus status;
  const StatusIndicator({required this.status, super.key});

  @override
  Widget build(BuildContext context) {
    final f = NumberFormat.decimalPercentPattern(decimalDigits: 2);
    return AnimatedContainer(
      key: const ValueKey('indi'),
      duration: const Duration(seconds: 1),
      alignment: Alignment.center,
      width: 100,
      margin: const EdgeInsets.all(4),
      padding: const EdgeInsets.symmetric(horizontal: 8),
      decoration: BoxDecoration(
        color: status.color,
        border: Border.all(),
        borderRadius: BorderRadius.circular(24),
      ),
      child: switch (status) {
        MachineStatus.generating => BlocBuilder<MachineBloc, MachineState>(
          buildWhen:
              (previous, current) =>
                  current.genProgress != previous.genProgress,
          builder:
              (context, state) =>
                  state.genProgress != 0.0
                      ? Text(f.format(state.genProgress))
                      : const Text(''),
        ),
        _ => Text(status.text),
      },
    );
  }
}

Code for the second example

class StatePanel extends StatelessWidget {
  const StatePanel({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocBuilder<MachineBloc, MachineState>(
      buildWhen: (previous, current) => current.status != previous.status,
      builder: (context, state) {
        return Row(
          children: [
            state.status.isRefreshable
                ? IconButton(
                  color: Colors.amber,
                  onPressed:
                      () => context.read<MachineBloc>().add(
                        WeatherRefreshRequested(),
                      ),
                  icon: const Icon(Icons.restart_alt),
                )
                : const IconButton(
                  color: Colors.grey,
                  onPressed: null,
                  icon: Icon(Icons.restart_alt),
                ),
            StatusIndicator(status: state.status),
          ],
        );
      },
    );
  }
}

[chris-NR]

@munificent I have tried the latest formatter (from main) and can confirm that the issue given in the first example above is resolved, thanks!

The second issue remains, here is the code (formatted with the latest version) - line break occurred previously after 'context'

class StatePanel extends StatelessWidget {
  const StatePanel({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocBuilder<MachineBloc, MachineState>(
      buildWhen: (previous, current) => current.status != previous.status,
      builder: (context, state) {
        return Row(
          children: [
            state.status.isRefreshable
                ? IconButton(
                    color: Colors.amber,
                    onPressed: () => context.read<MachineBloc>().add(
                      WeatherRefreshRequested(),
                    ),
                    icon: const Icon(Icons.restart_alt),
                  )
                : const IconButton(
                    color: Colors.grey,
                    onPressed: null,
                    icon: Icon(Icons.restart_alt),
                  ),
            StatusIndicator(status: state.status),
          ],
        );
      },
    );
  }
}

[munificent]

The second issue is much more subtle. Given a => expression containing a method call chain which in turn contains an argument list, there are multiple reasonable ways to format it:

onPressed: () => context.read<MachineBloc>().add(
  WeatherRefreshRequested(),
),

onPressed: () => context
    .read<MachineBloc>()
    .add(
      WeatherRefreshRequested(),
    ),

onPressed: () => context
    .read<MachineBloc>()
    .add(WeatherRefreshRequested()),

onPressed:
    () => context.read<MachineBloc>().add(WeatherRefreshRequested()),

onPressed:
    () =>
        context.read<MachineBloc>().add(
          WeatherRefreshRequested(),
        ),

// Etc.

There's no obvious answer as to which is clearly better. Even hand-formatted code varies in how this stuff is handled, sometimes withing a single file. A rule that prefers one over another here might end up making other similar code worse.

So the formatter has some rules and heuristics. They don't always do a perfect job, but my hope is that they're close enough to optimal that it's not worth sweating the difference.

But coming from the old formatter, yes, sometimes the rules are different in ways that lead to different formatting that isn't clearly better. That feels pointlessly disruptive. We tried to minimize that as much as possible with the rewrite, but there was only so much we could do. The new formatter is very different in its internal architecture, and perfect compatibility wasn't possible. (Or a goal, for that matter. After all, we rewrote it to produce better output than the old one could.)

[chris-NR]

Thanks for explaining the background to this issue and the implementation of the new formatter.

In the case where the new format is 'worse' or 'not obviously better' than the previous format, given the large number of developers who will be accustomed to the previous format, I think there should be a strong presumption in favour of the previous format (internal architecture permitting).

[munificent]

I think there should be a strong presumption in favour of the previous format (internal architecture permitting).

Agreed, totally. If I could have, I would have made the new formatter bug-for-bug compatible with the old one except in places where we want to deliberately fix obvious style bugs (which there were many) or where it implemented the taller argument list style.

But the internal architecture is so deeply different that that just wasn't possible. :-/

One way to think of the formatter is that it's sort of like a compiler, except a compiler where every single character of the code it generates is user visible behavior. That makes it really hard to refactor the internals in significant ways.