Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorrect indentation on chained method calls when the call is on the next line #1121

Closed
DetachHead opened this issue Jun 28, 2022 · 6 comments

Comments

@DetachHead
Copy link

before

void main() async {
  (await {
    'asdf': 'fdsa',
  }.toString())
      .foo(bar('baz'),);
}

after

expected

something like this

void foo() async {
  (await {
    'asdf': 'fdsa',
  }.toString())
      .foo(
          bar('baz'),
      );
}

actual

void foo() async {
  (await {
    'asdf': 'fdsa',
  }.toString())
      .foo(
    bar('baz'),
  );
}
@DanTup
Copy link
Contributor

DanTup commented Jul 7, 2022

I suspect this may be the same as the issue I came here to raise:

Future<void> a() {
  dynamic foo;
  return foo.aaaaaaaaaa
      .aaaaaaaaaa()
      .aaaaaaaaaa
      .aaaaaaaaaa
      .aaaaaaaaaa()
      .catchError((e) {
    print('');
  });
}

It's odd having catchError indented more than the function provided to it. Adding trailing commas changes things, but it still doesn't seem ideal:

Future<void> a() {
  dynamic foo;
  return foo.aaaaaaaaaa
      .aaaaaaaaaa()
      .aaaaaaaaaa
      .aaaaaaaaaa
      .aaaaaaaaaa()
      .catchError(
    (e) {
      print('');
    },
  );
}

@mengstr
Copy link

mengstr commented Jul 14, 2022

And maybe also what is causing this messy formatting for me...

class x {
  static void y() {
    http
        .post(
      Uri.parse('https://'),
      body: jsonEncode({}),
    )
        .then((value) {
      print("ok");
    }).catchError((error) {
      print("fail");
    });
  }
}

@antonshkurenko
Copy link

Hi everybody, I've come here to raise the same issue. In many other languages this simple case (indentation of closures) is handled nicely, but in dart it kills me (code changed because of NDA, but we have many many examples of it):

return getIt
      .getAsync<DbDao>()
      .asStream()
      .switchMap(
        (dao) => dao.fetchValues(
          id,
          id2,
          id3,
        ),
      )
      .map((values) => {for (var v in values) v.id: v})
      .distinct((prev, next) => mapEquals(prev, next))
      .map((values) {
    print("1"); // <- what is this 🥲
    print("2");
    for (var value in values) {
      print("3");
      while (value.next != null) {
        print("4");
        print("5");
      }
      print("6");
    }
    return values;
  });

@zedefi
Copy link

zedefi commented Apr 6, 2024

Same here, this is impossible to read, please provide some fix🥲

          ...
          return Stream.value(null)
              .flatMap(
            (value) => _client //???
                .apiUserLoginPost(
                  body: LoginRequest(email: email, password: password),
                )
                .asStream(),
          )
              .doOnData((response) {
            if (response.error != null) { //???
              throw Exception(response.error);
            }
          }).map((tokenResponse) {
            return tokenResponse.body!.token!;
          });

I have tried lines_longer_than_80_chars and require_trailing_commas, nothing seems to have an impact to this issue.

Desired result should be something like this:

          return Stream.value(null)
              .flatMap((value) =>
                _client.apiUserLoginPost(
                          body: LoginRequest(email: email, password: password),
                        )
                      .asStream(),
              )
              .doOnData((response) {
                  if (response.error != null) {
                    throw Exception(response.error);
                  }
              })
              .map((tokenResponse) {
                  return tokenResponse.body!.token!;
              });

@munificent
Copy link
Member

Yeah, I agree the examples in this issues all look pretty bad. They're a result of some limitations in how the formatter's internal representation currently works. (The short summary is that it is forced to decide how much indentation a piece of code will receive if it splits before it decides how or if it will split.)

This is one of the main reasons why we are moving to a new internal representation and a new style (#1253). With the new in-progress implementation, you get:

return Stream.value(null)
    .flatMap(
      (value) =>
          _client //???
              .apiUserLoginPost(
                body: LoginRequest(email: email, password: password),
              )
              .asStream(),
    )
    .doOnData((response) {
      if (response.error != null) {
        //???
        throw Exception(response.error);
      }
    })
    .map((tokenResponse) {
      return tokenResponse.body!.token!;
    });

@munificent
Copy link
Member

OK, now that the forthcoming tall style is further along, I ran all of these examples through it. Here's the output:

void main() async {
  (await {
        'asdf': 'fdsa', // comment to force split
      }.toString())
      .foo(
        bar('baz'), // comment to force split
      );
}

The indentation here is kind of strange because the formatter doesn't recognize an await expression containing a collection literal as being one of the special kinds of method call targets that gets block-like formatting. I could add that but... it never makes sense to await a collection literal anyway, so I don't think that would add much value.

Future<void> a() {
  dynamic foo;
  return foo.aaaaaaaaaa
      .aaaaaaaaaa()
      .aaaaaaaaaa
      .aaaaaaaaaa
      .aaaaaaaaaa()
      .catchError((e) {
        print('');
      });
}

This looks right to me.

class x {
  static void y() {
    http
        .post(Uri.parse('https://'), body: jsonEncode({}))
        .then((value) {
          print("ok");
        })
        .catchError((error) {
          print("fail");
        });
  }
}

This too.

return getIt
    .getAsync<DbDao>()
    .asStream()
    .switchMap((dao) => dao.fetchValues(id, id2, id3))
    .map((values) => {for (var v in values) v.id: v})
    .distinct((prev, next) => mapEquals(prev, next))
    .map((values) {
      print("1");
      print("2");
      for (var value in values) {
        print("3");
        while (value.next != null) {
          print("4");
          print("5");
        }
        print("6");
      }
      return values;
    });

And this.

        return Stream.value(null)
            .flatMap(
              (value) =>
                  _client
                      .apiUserLoginPost(
                        body: LoginRequest(email: email, password: password),
                      )
                      .asStream(),
            )
            .doOnData((response) {
              if (response.error != null) {
                throw Exception(response.error);
              }
            })
            .map((tokenResponse) {
              return tokenResponse.body!.token!;
            });

The indentation is a little severe inside the call to flatMap() but I think the resulting formatting is at least simple and clear. I've considered tweaking the rules around => functions inside argument lists to allow something more compact like:

            .flatMap((value) =>
                _client
                    .apiUserLoginPost(
                      body: LoginRequest(email: email, password: password),
                    )
                    .asStream(),
            )

But when I test that on a bigger corpus, it seems to make more code look worse than that it makes look better. So far, I've stuck with the simple rule that an => argument works like other arguments and forces the surrounding argument list to split if the => contains a newline.

So, overall, I think the new formatter addresses the examples in this issue the way I expect and looks a lot better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants