Weird indentation

[BlackHC]

Looks quite wrong/unreadable:

      return new Future.value(counterTracker)
          .then((_) => throw new TestException())
          .whenComplete(() {
        print(1);
      }).catchError((_) => {}, test: (e) => e is TestException);

[munificent]

This looks OK to me. There are lots of interesting questions about how to handle mixed chains of method calls and function bodies. I don't think there's any simple rules that work great in all cases, but I'm OK with the output here.

[BlackHC]

What do we have to do to convince you to change your opinion? ^^ This indentation clearly does not make sense to me and I'm quite sure breaks lots of established indentation guidelines in lots of other languages except dart, it seems. Ie how can it ever be a good idea to reduce the nesting for a subexpression?

So, what exactly are the steps I need to take to convince you otherwise? Should I create a survey and have a vote by people about stupid formatter results?

[munificent]

Ie how can it ever be a good idea to reduce the nesting for a subexpression?

test("this test has a long description"
    "that does not fit on one line", () {
  // Only indented +2.
});

All of the Dart world's unittests would explode if the formatter started indenting every function and collection literal using the expression nesting rules.

The formatter does apply expression nesting indentation to functions and collections in some cases, but the heuristics for it are fairly subtle and probably not optimal. I would definitely be up for tweaking them, but so far, I haven't figured out what to change them to. Every other heuristic I've tried has made more code worse than it makes better when you try it on a big corpus.

It may be that the formatter should be more open to indenting collections like expressions than functions. The latter is really where the extra indentation is painful because function bodies may be huge and contain their own long statements.

If you're curious, most of the relevant code is in argument_list_visitor.dart. If you have ideas for what guidelines should be used to tell when a body should be indented relative to the statement versus the expression, let me know.

[eernstg]

Andreas,

if you think the underlying rules for this indentation are incrutable it might be helpful to take the following rule of thumb into account: When a complex expression is formatted we use a 4 step indentation for "continuation lines" (where a single expression overflows into the next line); however, "large" nested {} blocks will be aligned with the expression as a whole (hence the zero-level indent of the })), and inside the {} block we will use the standard 2 step indentation.

Hence, it's not chaos, it's more like "ah, something starts here (line 1), but that's a large expression (lines 2, 3: continuation indent), and then we have a function literal given as an argument ({} block indent), and the rest fits nicely on one line".

It is indeed violating the AST structure to completely "dedent" that nested function body, but it's possible to get used to it, and you shouldn't write syntactic structures so deeply nested that it goes totally haywire, anyway! ;-)

[BlackHC]

Thanks for the explanations! I'll try to have a look!
On Fri 11 Sep 2015 at 10:15 Erik Ernst notifications@github.com wrote:

Andreas,

if you think the underlying rules for this indentation are incrutable it
might be helpful to take the following rule of thumb into account: When a
complex expression is formatted we use a 4 step indentation for
"continuation lines" (where a single expression overflows into the next
line); however, "large" nested {} blocks will be aligned with the
expression as a whole (hence the zero-level indent of the })), and inside
the {} block we will use the standard 2 step indentation.

Hence, it's not chaos, it's more like "ah, something starts here (line 1),
but that's a large expression (lines 2, 3: continuation indent), and then
we have a function literal given as an argument ({} block indent), and the
rest fits nicely on one line".

It is indeed violating the AST structure to completely "dedent" that
nested function body, but it's possible to get used to it, and you
shouldn't write syntactic structures so deeply nested that it goes totally
haywire, anyway! ;-)

—
Reply to this email directly or view it on GitHub
#415 (comment)
.

[BlackHC]

    return _jsonRequestHandler(requestName, data, background, mutating)
        .then((responseJson) {
      final response = _marshaller.deserialize(JSON.decode(responseJson));
      return Request.checkType(request, response);
    })
        .whenComplete(_pendingOperationCompleted)
        .onCancel(_pendingOperationCompleted);

I rest my case ;)

[munificent]

Yes, this is one of the difficult cases for the formatter to handle. Imagine the function passed to then() was a 100 lines long (a common occurrence). Would you really want the whole thing indented another four spaces just to make that .whenComplete() look better?

[BlackHC]

(Hmm, you could argue that you should probably try to use await/async in that case ^^)

I would prefer to have +2 within the nesting, which would be okay with the style guide I think:

    return _jsonRequestHandler(requestName, data, background, mutating)
        .then((responseJson) {
          final response = _marshaller.deserialize(JSON.decode(responseJson));
          return Request.checkType(request, response);
        })
        .whenComplete(_pendingOperationCompleted)
        .onCancel(_pendingOperationCompleted);

[munificent]

(Hmm, you could argue that you should probably try to use await/async in that case ^^)

Absolutely. Using async/await more definitely makes the formatter's job easier.

You're right that the style guide permits the function to be indented, and in some cases, the formatter will choose to do that. But it tries pretty hard to avoid it in most cases because it can cause pretty brutal code churn. Imagine you have some code like:

return _jsonRequestHandler(requestName, data, background, mutating)
    .then((responseJson) {
  final response = _marshaller.deserialize(JSON.decode(responseJson));
  return Request.checkType(request, response);

  // 200 more lines of code...      
}).whenComplete(_pendingOperationCompleted);

This is fine and everyone is happy. Then you later add that .onCancel() and now the stuff after the function doesn't fit in one line. It would be rough to push the entire function over +4 at that point, which in turn may cause lots of statements inside that function to get re-wrapped.

In general, the formatter doesn't try to minimize deltas, but with functions we try to be sensitive to the fact that most of the time they look best when indented relative to the statement.

[fmmarzoa]

Just my two cents here, as I'm facing some weird formatting with this too, just to not create a new issue since it seems totally related with this one.

                  _auth
                      .createUserWithEmailAndPassword(
                    email: _email!,
                    password: _password!,
                  )
                      .then((value) {
                    _showSpinner = false;
                    Navigator.pushNamed(context, ChatScreen.routeId);
                  }).onError((error, stackTrace) {
                    _showSpinner = false;
                    print("$error\n$stackTrace");
                  });

If I were to indent that manually myself, I'd have done something like this perhaps:

                  _auth.createUserWithEmailAndPassword(
                    email: _email!,
                    password: _password!,
                  ).then((value) {
                    _showSpinner = false;
                    Navigator.pushNamed(context, ChatScreen.routeId);
                  }).onError((error, stackTrace) {
                    _showSpinner = false;
                    print("$error\n$stackTrace");
                  });

It's just removing a couple of line feeds there that seem weirdly out of place to me.

[munificent]

Some of the weirdness here is coming from the trailing commas. If you remove that, you get:

                  _auth
                      .createUserWithEmailAndPassword(
                          email: _email!, password: _password!)
                      .then((value) {
                    _showSpinner = false;
                    Navigator.pushNamed(context, ChatScreen.routeId);
                  }).onError((error, stackTrace) {
                    _showSpinner = false;
                    print("$error\n$stackTrace");
                  });

See: #1008.

[fmmarzoa]

I actually added the commas to see if it looked any better, but I didn't see much of a difference. I'll check that other issue, thanks!