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

move "(" and ")" to next lines if callee is parenthesized cascade sequence #549

Closed
skybrian opened this issue Oct 31, 2016 · 8 comments
Closed

Comments

@skybrian
Copy link

"At this time, we do not recommend using dartfmt, as it greatly decreases the readability of components built using OverReact's fluent-style." [1]

Seems unfortunate.

[1] https://github.com/Workiva/over_react#component-formatting

@matanlurey
Copy link
Contributor

I wonder how much of this was prior to trailing comma support that made Flutter happy.

@greglittlefield-wf
Copy link

greglittlefield-wf commented Nov 2, 2016

over_react maintainer here!

Wow, I hadn't checked in in awhile, but it looks like the trailing commas make a world of difference!!

  • dart_style, without trailing commas:

    render() {
      return (Dom.div()..className = 'brand-img-wrapper')(
          (Dom.div()
            ..className = 'brand-img brand-symbol'
            ..role = 'img'
            ..addProps(ariaProps()..hidden = true))(),
          (Dom.button()
            ..type = 'button'
            ..className = 'hitarea sidebar-toggle'
            ..onClick = _onSidebarToggle)((Dom.span()..className = 'flip-container')((Dom.span()..className = 'flipper')(
              (Dom.span()..className = 'front-side')((Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_RIGHT)()),
              (Dom.span()..className = 'back-side')((Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_LEFT)())))));
    }
  • dart_style, with trailing commas:

    render() {
      return (Dom.div()..className = 'brand-img-wrapper')(
        (Dom.div()
          ..className = 'brand-img brand-symbol'
          ..role = 'img'
          ..addProps(ariaProps()..hidden = true))(),
        (Dom.button()
          ..type = 'button'
          ..className = 'hitarea sidebar-toggle'
          ..onClick = _onSidebarToggle)(
          (Dom.span()..className = 'flip-container')(
            (Dom.span()..className = 'flipper')(
              (Dom.span()..className = 'front-side')(
                (Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_RIGHT)(),
              ),
              (Dom.span()..className = 'back-side')(
                (Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_LEFT)(),
              ),
            ),
          ),
        ),
      );
    }

The only thing we'd want improved would be to wrap the first closing paren on its own line so that the prop cascading and nesting are more visually distinguished:

  • Ideal formatting:

    render() {
      return (Dom.div()..className = 'brand-img-wrapper')(
        (Dom.div()
          ..className = 'brand-img brand-symbol'
          ..role = 'img'
          ..addProps(ariaProps()..hidden = true)
        )(),
        (Dom.button()
          ..type = 'button'
          ..className = 'hitarea sidebar-toggle'
          ..onClick = _onSidebarToggle
        )(
          (Dom.span()..className = 'flip-container')(
            (Dom.span()..className = 'flipper')(
              (Dom.span()..className = 'front-side')(
                (Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_RIGHT)(),
              ),
              (Dom.span()..className = 'back-side')(
                (Icon()..glyph = IconGlyph.CHEVRON_DOUBLE_LEFT)(),
              ),
            ),
          ),
        ),
      );
    }

That being said, this is a _fantastic_ improvement over the previous formatting; I'm very excited to share this news!

@munificent
Copy link
Member

I'm not sure if we could pull off your ideal formatting without making other code that doesn't use this idiom—a parenthesized expression followed by an argument list—for the same reason look worse.

@greglittlefield-wf
Copy link

What about if it were only done for parenthesized cascade expressions? 😃

@munificent
Copy link
Member

No, those are actually sadly common in practice. Despite the fact that the designers deliberately chose a cascade syntax that doesn't nest well, users nest it all the time.

If we changed how those are formatted, it would impact tons of existing code.

@greglittlefield-wf
Copy link

Mm, fair enough. Thanks for giving it some thought!

I'll have to familiarize myself with the different ways cascading are used to see if I can't come up with something.

@munificent munificent changed the title over_react recommends not to use dartfmt move "(" to next line if callee is parenthesized cascade sequence Nov 8, 2016
@greglittlefield-wf
Copy link

@munificent Just saw the updated title. To be clear, ideally we'd like to move closing paren to the next line as well.

@munificent munificent changed the title move "(" to next line if callee is parenthesized cascade sequence move "(" and ")" to next lines if callee is parenthesized cascade sequence Jan 3, 2017
@munificent
Copy link
Member

I think we're happy with the way parenthesized cascades are currently formatted.

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

4 participants