dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
646 stars 120 forks source link

Tall style: Keep => closer to the function call for one-arg functions #1532

Open nex3 opened 2 months ago

nex3 commented 2 months ago

Generally, for a single-argument function that takes a callback, I expect the callback argument to be tightly bound to the call site. For example, I get:

void main() {
  children?.any(
    (child) => switch (child) {
      VariableDeclaration() || FunctionRule() || MixinRule() => true,
      ImportRule(:var imports) => imports.any(
        (import) => import is DynamicImport,
      ),
      _ => false,
    },
  );
}

where I would expect

void main() {
  children?.any((child) => switch (child) {
    VariableDeclaration() || FunctionRule() || MixinRule() => true,
    ImportRule(:var imports) => imports.any(
      (import) => import is DynamicImport,
    ),
    _ => false,
  });
}

although even

void main() {
  children?.any((child) =>
    switch (child) {
      VariableDeclaration() || FunctionRule() || MixinRule() => true,
      ImportRule(:var imports) => imports.any(
        (import) => import is DynamicImport,
      ),
      _ => false,
    },
  );
}

would be preferable.

I similarly dislike cases like

void main() {
  complex.components.any(
    (component) =>
        component.combinators.length > 1 || component.selector.accept(this),
  );
}

which I would prefer to have as

void main() {
  complex.components.any((component) =>
      component.combinators.length > 1 || component.selector.accept(this),
  );
}

(This is also more consistent with how the same function call would be structured if it used a bracketed callback, which to my mind is a benefit.)

rrousselGit commented 2 months ago

I similarly dislike cases like

void main() {
  complex.components.any(
    (component) =>
        component.combinators.length > 1 || component.selector.accept(this),
  );
}

which I would prefer to have as

void main() {
  complex.components.any((component) =>
      component.combinators.length > 1 || component.selector.accept(this),
  );
}

I don't like that. It feels odd to me that a trailing comma after a parameter wouldn't cause the parameter to start on a newline.

The way I'd deal with those specific cases is to not use => if it cannot be written in a single line.

So I'd do:

void main() {
  complex.components.any((component) {
    return component.combinators.length > 1 || component.selector.accept(this);
  });
}

IMO switching between => and {} is like adding/removing a trailing comma. It's one of my core formatting tools.

nex3 commented 2 months ago

The point of adding the tall style is that commas don't cause anything. I'd be just as happy with the last example being

void main() {
  complex.components.any((component) =>
      component.combinators.length > 1 || component.selector.accept(this)
  );
}

which matches a single-argument function with a block callback not having a comma.

IMO switching between => and {} is like adding/removing a trailing comma. It's one of my core formatting tools.

I don't want "formatting tools", I want an opinionated formatter that ends debates about style issues in code reviews. If I get a code review comment like "if you make this a bracketed function it'll get the formatter to reduce indentation" that's purely negative value. I'd prefer that the formatter be in charge of choosing between => and {}, but failing that I'd like the two to be formatted similarly.

rrousselGit commented 2 months ago

Without a comma there, I don't mind as much.

I don't want "formatting tools", I want an opinionated formatter that ends debates about style issues in code reviews. If I get a code review comment like "if you make this a bracketed function it'll get the formatter to reduce indentation" that's purely negative value. I'd prefer that the formatter be in charge of choosing between => and {}, but failing that I'd like the two to be formatted similarly.

You can achieve the exact same thing with lint rules instead of baking it in the formatter.

Formatting the file already can apply quick fixes, so it'd remove unused imports, along with fixing whatever style issue you have based on your lints.

IMO having the formatter do it wlll be hurtful:

A formatter cannot fix the problem either. Style issues is also things like "early return VS if/else" or "for VS while" or "method tearoff VS local closure" or folder architecture, ect...

That's way out of the scope of the formatter. And not fixing those means style issues are still a thing. But we could have a lint rule for every single one of those.

lrhn commented 2 months ago

If I understand the logic, the formatting without a trailing comma would be:

void main() {
  complex.components.any((component) =>
      component.combinators.length > 1 || component.selector.accept(this));
}

if that is at all possible with an argument taking more than one line, and

void main() {
  complex.components.any(
      (component) =>
          component.combinators.length > 1 || component.selector.accept(this),
  );
}

with a trailing comma.comma (each argument on its own line(s)).

(Making argument lists "tall" is an anti-goal for me. I want my code as I want my gmail, "compact"!)