dartfmt should recognize off/on delimiter comment pairs to temporarily disable reformatting

[tlarsen4google]

Since no formatter is perfect (e.g., some const/final nested collection constructs try to represent actual readable structure meaning in the code via their indentation that the formatter cannot be expected to understand and preserve), formatter off/on comment pairs should be recognized that turn off reformatting for the specified lines in between these delimiters.

// dartfmt off
Foo barBaz() => new Quux({
    'alpha.beta.gamma': [new Zuul({
      'yarrr': Abracadabra.OPEN_SESAME,
    }, {
    'toil': {
      'trouble': {
        'fireBurn': {
            'cauldronBubble': EYE_OF_NEWT,
        }
      }
    }
  })]
});
// dartfmt on

Yes, use of this construct should be discouraged and limited. Perhaps emit a warning as dartfmt is run indicating that it encountered such blocks of unformatted code?

Also, it is only reasonable to require complete constructs be present between the delimiters, and not expect the formatter to do anything reasonable when the delimiters arbitrarily break up the middle of some block level construct. For example, the formatter could produce an error, or just not be expected to do anything sane, for misuses like:

  if (someTest) {
    someThenCode();
// dartfmt off
               unformattedThenCode();
} else {
          unformattedElseCode();
// dartfmt on
    moreElseCode();
  }

At worst, such unformatted segments that span block boundaries should produced undefined results for the rest of the source file. At best, an error should be printed and formatting should stop.

[munificent]

I understand the desire here, but it goes against the fundamental goals of the formatter. Its goals, in order of descending priority, are:

1. Produce consistently formatted code.
2. End debates about style issues in code reviews.
3. Free users from having to think about formatting.
4. Produce beautiful, readable output that helps users understand the intent and structure of the code.

Point number 4 is critical because if the formatter isn't good enough, people simply won't use it. In some cases, though, it can't do as good a job as a human can, because a human has semantic knowledge, like "split here to keep these two arguments together since they form a logical pair".

A "let the human do this" annotation would give an escape hatch for those cases. Unfortunately, it does so at the expect of the first three goals. As soon as you have these annotations, you're right back to quibbling about how the code inside them should be formatted. Worse, you now also open the metargument about where in the code you should be allowed to use the annotations. I certainly don't want to start more style debates!

The set of places where a human could better than the formatter shrinks with every release. Note that I use "could" deliberately here. While a human can in theory do a better job hand-formatting sometimes, we're also error-prone.

Your carefully hand-formattted example has two erroneous spaces before 'cauldronBubble'. Today, the formatter fixes that error and formats your example to:

Foo barBaz() => new Quux({
      'alpha.beta.gamma': [
        new Zuul({
          'yarrr': Abracadabra.OPEN_SESAME,
        }, {
          'toil': {'trouble': {'fireBurn': {'cauldronBubble': EYE_OF_NEWT,}}}
        })
      ]
    });

Personally, I prefer that over your hand formatting. If we take your suggestion on #223 (which I like but I need to see how it plays out in practice), you get:

Foo barBaz() => new Quux({
      'alpha.beta.gamma': [
        new Zuul({
          'yarrr': Abracadabra.OPEN_SESAME,
        }, {
          'toil': {
            'trouble': {
              'fireBurn': {'cauldronBubble': EYE_OF_NEWT,}
            }
          }
        })
      ]
    });

I think that's the best formatting of all.

Finally—and this is the least important motivation—not having an escape hatch keeps me honest. Since the formatter will touch all of your code, it forces me to work very hard to get it to do so in a way you like. If you could just turn off formatting, it would be easy for me to close bugs with just, "Eh, just don't format that bit."

[tlarsen4google]

Sorry, but what is actually happening is that our team is simply not running dartfmt on entire files because they have special huge (512 entries) byte tables in Lists that are getting expanded to ridiculous single item per line reformats.

So, lack of this feature is causing entire files, with lots of other code that could benefit from the standardization of Dart formatting, to completely be hand-formatted because of regions in the file that dartfmt will never be able to handle properly.

[tlarsen4google]

Perhaps there at least need to be ways to control the aggressiveness of dartfmt?
E.g. something like this (though I'm just making up the format as an example and don't care what it actually ends up looking like):

// dartfmt: compact structured literals
...
// dartfmt: reset to defaults

// dartfmt: expanded structured literals
...
// dartfmt: reset to defaults

My point is that there are places where dartfmt actually harms the readability of the resulting code, and both possibilities are still legal Dart style results.

[tlarsen4google]

Real world example:

  /**
   * Basic one-byte 608 CC char set, mostly ASCII.
   * Indexed by (char-0x20).
   */
  static const List<int> ccUtfTable0 = const <int> [
    0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27,     //   ! " # $ % & '
    0x28, 0x29,                                         // ( )
    0xE1,       // 2A: 225 'á' "Latin small letter A with acute"
    0x2b, 0x2c, 0x2d, 0x2e, 0x2f,     //       + , - . /
    0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,     // 0 1 2 3 4 5 6 7
    0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f,     // 8 9 : ; < = > ?
    0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47,     // @ A B C D E F G
    0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f,     // H I J K L M N O
    0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57,     // P Q R S T U V W
    0x58, 0x59, 0x5a, 0x5b,                             // X Y Z [
    0xE9,       // 5C: 233 'é' "Latin small letter E with acute"
    0x5d,                                               //           ]
    0xED,       // 5E: 237 'í' "Latin small letter I with acute"
    0xF3,       // 5F: 243 'ó' "Latin small letter O with acute"
    0xFA,       // 60: 250 'ú' "Latin small letter U with acute"
    0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67,     //   a b c d e f g
    0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f,     // h i j k l m n o
    0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77,     // p q r s t u v w
    0x78, 0x79, 0x7a,                                   // x y z
    0xE7,       // 7B: 231 'ç' "Latin small letter C with cedilla"
    0xF7,       // 7C: 247 '÷' "Division sign"
    0xD1,       // 7D: 209 'Ñ' "Latin capital letter N with tilde"
    0xF1,       // 7E: 241 'ñ' "Latin small letter N with tilde"
    0x25A0      // 7F:         "Black Square" (NB: 2588 = Full Block)
  ];

[lyceel]

Regarding objective 2 above, I can tell you that on our team, the introduction of dartfmt has created a great deal of debate over style issues in code reviews. There have been improvements, and we can live with some of the idiosyncrasies. The consistency and automatic nature are going to be worth most of the minor niggles going forward.

However, we really do need a way to preserve format on specific constructs like Todd's example above. All of the humans on the team agree that the "correct" format is the way that it is formatted above. The only disagreement comes from dartfmt. It's for cases like this that we need a way to overrule dartfmt.

[EmmanuelOga]

This would be a useful addition IMO.

I had a similar need when working with a different language that also included a formatter. The code had static Strings for specifying SQL queries. In Dart, that code would look something like this:

class Something {
  static String TableName = "SomeTable";

  // dartfmt off
  static String query1 = "SELECT * FROM ${TableName}"
                        " WHERE Blah Blah"
                        " HAVING Blah Blah";

  static String query2 = "SELECT * FROM ${TableName}"
                        " WHERE Blah Blah";
  // dartfmt on
}

While I was happy with the standard auto-formatting all across the app, I used some special alignment for the SQL queries that helped a big deal to make the embedded SQL queries readable.

If I hadn't had a way to turn off the formatter on sections of the file, my custom formatting would had been impossible.

[munificent]

So, lack of this feature is causing entire files, with lots of other code that could benefit from the standardization of Dart formatting, to completely be hand-formatted because of regions in the file that dartfmt will never be able to handle properly.

It sounds to me like the poor formatting of list literals is causing you to not use dartfmt, not the ability to prevent certain areas from being formatted.

In this example, I believe #139 would solve your problem.

I used some special alignment for the SQL queries that helped a big deal to make the embedded SQL queries readable.

Here, I think putting the space on the previous string literal and then lining up the adjacent strings would be enough to get what you want, right?

[tlarsen4google]

No, #139 is not sufficient. Please see the closed captioning 608 character set table I added. dartfmt would have to simply leave that section alone for information to not be lost.

[EmmanuelOga]

@munificent my SQL code was just an example. The point is that sometimes you can't make the formatter work for your content.

Sometimes your want to embed content that, even when expressed as Dart code, can be made more readable when formatted using rules other than the default dartfmt ones.

[lyceel]

The List literal above is just one example. What if this particular case didn't have comments after each
line? What if we had a Map literal, or an entire JSON-like structure that was formatted in a non-standard way for enhanced readability? Is it really possible to have a special case for every contingency?

So far, the argument against this idea has been that it makes it too easy for someone to turn off the formatter at the top of the file and avoid formatting. In this case, why would they bother to run the formatter at all? In an engineering team setting, either the team decides to use the formatter or they don't. If a member of a team that has adopted the formatter decides to turn off formatting for a file, they'd have to justify this to the rest of the team (via a code review or something similar). If the team agrees that it's the right thing to do, why should dartfmt not give them that choice?

The argument to not support such a control seems arbitrary to me. A good tool leaves decisions like this up to the developers that use it. The clang-format tool is one example that provides this option: http://goo.gl/mipb9i

[munificent]

Please see the closed captioning 608 character set table I added.

Your example was what I was referring to when I said #139 would probably fix it. I have a local implementation of that and it outputs:

class Foo {
  /**
   * Basic one-byte 608 CC char set, mostly ASCII.
   * Indexed by (char-0x20).
   */
  static const List<int> ccUtfTable0 =
      const <int>[
    0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, //   ! " # $ % & '
    0x28, 0x29, // ( )
    0xE1, // 2A: 225 'á' "Latin small letter A with acute"
    0x2b, 0x2c, 0x2d, 0x2e, 0x2f, //       + , - . /
    0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, // 0 1 2 3 4 5 6 7
    0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, // 8 9 : ; < = > ?
    0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, // @ A B C D E F G
    0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, // H I J K L M N O
    0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, // P Q R S T U V W
    0x58, 0x59, 0x5a, 0x5b, // X Y Z [
    0xE9, // 5C: 233 'é' "Latin small letter E with acute"
    0x5d, //           ]
    0xED, // 5E: 237 'í' "Latin small letter I with acute"
    0xF3, // 5F: 243 'ó' "Latin small letter O with acute"
    0xFA, // 60: 250 'ú' "Latin small letter U with acute"
    0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, //   a b c d e f g
    0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f, // h i j k l m n o
    0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, // p q r s t u v w
    0x78, 0x79, 0x7a, // x y z
    0xE7, // 7B: 231 'ç' "Latin small letter C with cedilla"
    0xF7, // 7C: 247 '÷' "Division sign"
    0xD1, // 7D: 209 'Ñ' "Latin capital letter N with tilde"
    0xF1, // 7E: 241 'ñ' "Latin small letter N with tilde"
    0x25A0 // 7F:         "Black Square" (NB: 2588 = Full Block)
  ];
}

It could be better if it lined up the line comments (by implementing #48), but it would also be easy to just tweak the spacing after the //:

class Foo {
  /**
   * Basic one-byte 608 CC char set, mostly ASCII.
   * Indexed by (char-0x20).
   */
  static const List<int> ccUtfTable0 =
      const <int>[
    0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, //       ! " # $ % & '
    0x28, 0x29, //                                         ( )
    0xE1, //    2A: 225 'á' "Latin small letter A with acute"
    0x2b, 0x2c, 0x2d, 0x2e, 0x2f, //                             + , - . /
    0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, //     0 1 2 3 4 5 6 7
    0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, //     8 9 : ; < = > ?
    0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, //     @ A B C D E F G
    0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, //     H I J K L M N O
    0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, //     P Q R S T U V W
    0x58, 0x59, 0x5a, 0x5b, //                             X Y Z [
    0xE9, //    5C: 233 'é' "Latin small letter E with acute"
    0x5d, //                                                         ]
    0xED, //    5E: 237 'í' "Latin small letter I with acute"
    0xF3, //    5F: 243 'ó' "Latin small letter O with acute"
    0xFA, //    60: 250 'ú' "Latin small letter U with acute"
    0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, //           a b c d e f g
    0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f, //     h i j k l m n o
    0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, //     p q r s t u v w
    0x78, 0x79, 0x7a, //                                   x y z
    0xE7, //    7B: 231 'ç' "Latin small letter C with cedilla"
    0xF7, //    7C: 247 '÷' "Division sign"
    0xD1, //    7D: 209 'Ñ' "Latin capital letter N with tilde"
    0xF1, //    7E: 241 'ñ' "Latin small letter N with tilde"
    0x25A0 //   7F:         "Black Square" (NB: 2588 = Full Block)
  ];
}

I don't want to sound cheeky here, but I will also point out that your original carefully hand-formatted example has two formatting errors in it.

What if this particular case didn't have comments after each
line?

Well, you could add them. If you want your code to be consistently formatted and tuned to your liking, the easiest way is to let the formatter do the dirty work and work with it by tweaking your code a bit so that its output looks how you want.

What if we had a Map literal, or an entire JSON-like structure that was formatted in a non-standard way for enhanced readability? Is it really possible to have a special case for every contingency?

No, I try to avoid special case rules. What I like are general rules that solve a large number of cases. Here, the general rule seems to be that large collection literals often have some internal structure that the author wants to highlight. Letting the formatter preserve that seems reasonable to me.

There aren't an infinite number of contingencies. All we have to do is look at the world's actual Dart code and see how the formatter handles it. I'm sure it will produce weird output it, I don't know, you make all of your identifiers two letters and give every class twenty type parameters. But, in practice, people don't do that, so the formatter doesn't have to optimize for that kind of code.

So far, the argument against this idea has been that it makes it too easy for someone to turn off the formatter at the top of the file and avoid formatting.

No, the argument is that disabling formatting makes code less consistent, which is the primary goal of the formatter.

If the team agrees that it's the right thing to do, why should dartfmt not give them that choice?

It does: just don't pass that file to the list of files you want it to format.

A good tool leaves decisions like this up to the developers that use it. The clang-format tool is one example that provides this option.

clang-format has different goals and—more importantly—different constraints than the Dart formatter. When you're trying to be a useful tool for the millions of C/C++ developers maintaining billions of lines of code written under a variety of style guides over the past 50 years, yeah, you have to be configurable.

But, if you're a new language that is trying to improve the consistency of a relatively small corpus of code, configuration hinders your goals. gofmt has no configurability nor any ability to disable it on sections of code.

[tlarsen4google]

Ah, but #golang takes a very hard line by:

• having no compiler warnings at all (if it is worth output, it is an error)
  (compared with Dart having problems, warnings, and hints)
• enforcing certain style decisions at the language level (e.g. placement of { and })
  (versus if (foo) { bar = baz; } being perfectly valid in Dart, from a compiler perspective)

I don't see Dart ever heading in that strictness direction, so I don't think the comparison to gofmt is completely apt. Is the strictness of #golang an actual goal for Dart?

[munificent]

I don't see Dart ever heading in that strictness direction, so I don't think the comparison to gofmt is completely apt. Is the strictness of #golang an actual goal for Dart?

I can't speak for the entire language, but when it comes to style, yes, it is. It's not that we want to be strict it's that we want to reduce the set of things you have to do manually to zero.

[AldoMX]

I reached here trying to copy the examples from ColorFilter:
https://api.flutter.dev/flutter/dart-ui/ColorFilter/ColorFilter.matrix.html

My workaround was to add a comment at the end of the matrices, although I would had liked to preserve the padding between elements:

  static const _invertMatrix = <double>[
    -1, 0, 0, 0, 255,
    0, -1, 0, 0, 255,
    0, 0, -1, 0, 255,
    0, 0, 0, 1, 0,
    //
  ];
  static const _sepiaMatrix = <double>[
    0.393, 0.769, 0.189, 0, 0,
    0.349, 0.686, 0.168, 0, 0,
    0.272, 0.534, 0.131, 0, 0,
    0, 0, 0, 1, 0,
    //
  ];
  static const _grayscaleMatrix = <double>[
    0.2126, 0.7152, 0.0722, 0, 0,
    0.2126, 0.7152, 0.0722, 0, 0,
    0.2126, 0.7152, 0.0722, 0, 0,
    0, 0, 0, 1, 0,
    //
  ];

[aktxyz]

soooo much better to add random blank comments to poke the formatter into doing something less annoying ... than to be able to turn on/off formatting

[munificent]

Casting a resurrection spell!

After many many discussions over the years, and recent aspirations to get the Flutter repositories using dart format and the upcoming style (#1253), I'm persuaded that it's an overall net win to support opting out regions of code from formatting.

I still believe that it's a feature that should be rarely used. And, certainly, if you find yourself arguing with someone in a code review about whether or not a given region should be hand- or automatically formatted, there are more valuable things you could be doing with your life.

But for some kinds of code with complex structure, it may make sense to be able to, um, artisanally format it. And, in order to ease the migration to the new style (or other potential future style changes), it might be worth being able to control which parts of a program get reformatted and when.

It's yet to be determined what the markers to opt in or out would look like, or how opted out code interacts with the surrounding indentation.

[Hixie]

At the risk of starting a bike shed argument... The current convention is to put trailing // on a line to trigger as much of a disabling of the formatter as it currently allows (largely this is used to control where line breaks happen in lists, if I understand correctly). This seems both intuitive (as demonstrated by people already doing it) and very low-profile (certainly much less disruptive than the // clang-format off style used by LLVM). For example (taking examples from one of the issues linked above):

static const List<int> RAYS = [
     17,  0,  0,  0,  0,  0,  0, 16,  0,  0,  0,  0,  0,  0, 15, 0, //
      0, 17,  0,  0,  0,  0,  0, 16,  0,  0,  0,  0,  0, 15,  0, 0, //
      0,  0, 17,  0,  0,  0,  0, 16,  0,  0,  0,  0, 15,  0,  0, 0, //
      0,  0,  0, 17,  0,  0,  0, 16,  0,  0,  0, 15,  0,  0,  0, 0, //
      0,  0,  0,  0, 17,  0,  0, 16,  0,  0, 15,  0,  0,  0,  0, 0, //
      0,  0,  0,  0,  0, 17,  0, 16,  0, 15,  0,  0,  0,  0,  0, 0, //
      0,  0,  0,  0,  0,  0, 17, 16, 15,  0,  0,  0,  0,  0,  0, 0, //
      1,  1,  1,  1,  1,  1,  1,  0, -1, -1,  -1,-1, -1, -1, -1, 0, //
      0,  0,  0,  0,  0,  0,-15,-16,-17,  0,  0,  0,  0,  0,  0, 0, //
      0,  0,  0,  0,  0,-15,  0,-16,  0,-17,  0,  0,  0,  0,  0, 0, //
      0,  0,  0,  0,-15,  0,  0,-16,  0,  0,-17,  0,  0,  0,  0, 0, //
      0,  0,  0,-15,  0,  0,  0,-16,  0,  0,  0,-17,  0,  0,  0, 0, //
      0,  0,-15,  0,  0,  0,  0,-16,  0,  0,  0,  0,-17,  0,  0, 0, //
      0,-15,  0,  0,  0,  0,  0,-16,  0,  0,  0,  0,  0,-17,  0, 0, //
    -15,  0,  0,  0,  0,  0,  0,-16,  0,  0,  0,  0,  0,  0,-17, 0, //
  ];

const [4580,4470, 630,520, 630,520, 580,1670, 630,520, 580,1670, 630,520, 580,1670, 630,520, 630,1620, 630,520, 580,570, 580,520, 630,1670, 580,520, 630,1670, 580,520, 680,1620, 580,1670, 630,520, 580,520, 630,520, 630,520, 580,570, 580,520, 630,520, 580,570, 580,570, 580,1670, 580,570, 580,1670, 580,570, 580,1670, 580]; //