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

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

Open
nex3 opened this issue Aug 16, 2024 · 4 comments
Open

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

nex3 opened this issue Aug 16, 2024 · 4 comments

Comments

@nex3
Copy link
Member

nex3 commented Aug 16, 2024

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.)

@nex3 nex3 changed the title Tall style: Keep => closer to the function call Tall style: Keep => closer to the function call for one-arg functions Aug 16, 2024
@rrousselGit
Copy link

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
Copy link
Member Author

nex3 commented Aug 17, 2024

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
Copy link

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:

  • The style rules can be very complex. There will be too many false positives. A lint with false positives can be disabled. But disabling the formatter is extremely cumbersome.
  • Style guides change faster than the formatter and may be context dependent.
    Flutter code has different rules than Dart code.

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
Copy link
Member

lrhn commented Aug 27, 2024

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"!)

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

No branches or pull requests

3 participants