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

More polished select chains #444

Closed
olafurpg opened this Issue Sep 17, 2016 · 5 comments

Comments

2 participants
@olafurpg
Member

olafurpg commented Sep 17, 2016

The current select chain rules are pretty ad-hoc at the moment and badly documented. Considering select chains are really important in Scala I think they deserve some rethinking.

They rules currently work like this:

  • never line break before a dot . unless the dot is part of a breakable select chain.
  • A select chain becomes breakable at the first select dot . that leads an application that has parentheses () or square brackets [].
  • Either all dots in a breakable select chain get a line break or none of the dots get a line break.
  • By default, scalafmt puts the entire select chain on a single line if the chain can fit on a single line. PR #440 added a flag keepSelectChainLineBreaks that changes the default behavior so that scalafmt will put line breaks before all dots in a breakable select chain if there is a newline before the first dot in the breakable part of the select chain.

Some examples.

  • a really long sequence of foo.bar.kaz.man.kazam won't line break since there is no application with [] or () to make the chain breakable. This might mean that the formatted output exceeds the column limit.
  • in lst.head.foo(...).kaz the chain becomes breakable at .foo and not .head.
  • the following example never becomes breakable because the .collect uses curly braces
[info]   val cols = flatSchema.getFields.collect {
[info]     case f: PrimitiveType =>
[info]       Column(f.getName, typeMap(f, widths))
[info]   }.toList
  • keepSelectChainLineBreaks is not triggered on the following example because there is no newline before .from().
 val serializedBsonByKey= TypedPipe.from()
    .map{Base64.decodeBase64}
    .groupBy{key}
    .head
// scalafmt default output
val serializedBsonByKey =
  TypedPipe.from().map { Base64.decodeBase64 }.groupBy { key }.head
// scalafmt output with newline before .from and keepSelectChainLineBreaks enabled
val serializedBsonByKey = TypedPipe
  .from()
  .map { Base64.decodeBase64 }
  .groupBy { key }
  .head

Some questions

  1. Should curly braces trigger the start of a breakable select chain just like [] and () do? I remember I tried this out and for some cases it wasn't so great.
  2. What should the behavior of keepSelectChainLineBreaks actually be? Should the option be renamed?
  3. More generally, how do you think select chains should format?

The current rules are derived from looking at quite a lot code and any changes to the rules will probably require looking at more diffs. I personally quite like the default behavior except in some cases I think scalafmt should line break select chain even if the chains can fit on a single line.

cc/ @johnynek @melrief

@johnynek

This comment has been minimized.

Show comment
Hide comment
@johnynek

johnynek Sep 18, 2016

Contributor

I think the default behavior for keepSelectChainLineBreaks should be that
you only add new lines (never remove them) and you only add new lines in
order to satisfy max line width restrictions.

To the style we use with scalding (at Twitter and Stripe), .method { } is
a fine place to break. Side note, I don't think we should always break at
=> for instance .map { x => (x, x) } but that is another issue probably.
On Sat, Sep 17, 2016 at 07:03 Ólafur Páll Geirsson notifications@github.com
wrote:

The current select chain rules are pretty ad-hoc at the moment and badly
documented https://olafurpg.github.io/scalafmt/#Selectchains.
Considering select chains are really important in Scala I think they
deserve some rethinking.

They rules currently work like this:

  • never line break before a dot . unless the dot is part of a
    breakable select chain.
  • A select chain becomes breakable at the first select dot . that
    leads an application that has parentheses () or square brackets [].
  • Either all dots in a breakable select chain get a line break or none
    of the dots get a line break.
  • By default, scalafmt puts the entire select chain on a single line
    if the chain can fit on a single line. PR #440
    https://github.com/olafurpg/scalafmt/pull/440 added a flag
    keepSelectChainLineBreaks that changes the default behavior so that
    scalafmt will put line breaks before all dots in a breakable select chain
    if there is a newline before the first dot in the breakable part of
    the select chain.

Some examples.

  • a really long sequence of foo.bar.kaz.man.kazam won't line break
    since there is no application with [] or () to make the chain
    breakable. This might mean that the formatted output exceeds the column
    limit.
  • in lst.head.foo(...).kaz the chain becomes breakable at .foo and not
    .head.
  • the following example never becomes breakable because the .collect
    uses curly braces

[info] val cols = flatSchema.getFields.collect {
[info] case f: PrimitiveType =>
[info] Column(f.getName, typeMap(f, widths))
[info] }.toList

  • keepSelectChainLineBreaks is not triggered on the following example
    because there is no newline before .from().

    val serializedBsonByKey= TypedPipe.from()
    .map{Base64.decodeBase64}
    .groupBy{key}
    .head// scalafmt default outputval serializedBsonByKey =
    TypedPipe.from().map { Base64.decodeBase64 }.groupBy { key }.head// scalafmt output with newline before .from and keepSelectChainLineBreaks enabledval serializedBsonByKey = TypedPipe
    .from()
    .map { Base64.decodeBase64 }
    .groupBy { key }
    .head

Some questions

  1. Should curly braces trigger the start of a breakable select chain
    just like [] and () do? I remember I tried this out and for some cases
    it wasn't so great.
  2. What should the behavior of keepSelectChainLineBreaks actually be?
    Should the option be renamed?
  3. More generally, how do you think select chains should format?

The current rules are derived from looking at quite a lot code and any
changes to the rules will probably require looking at more diffs. I
personally quite like the default behavior except in some cases I think
scalafmt should line break select chain even if the chains can fit on a
single line.

cc/ @johnynek https://github.com/johnynek @melrief
https://github.com/melrief


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/olafurpg/scalafmt/issues/444, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdlxT1-Smqp5gxmu5apwdUNF_hGgXks5qrB1bgaJpZM4J_ph0
.

Contributor

johnynek commented Sep 18, 2016

I think the default behavior for keepSelectChainLineBreaks should be that
you only add new lines (never remove them) and you only add new lines in
order to satisfy max line width restrictions.

To the style we use with scalding (at Twitter and Stripe), .method { } is
a fine place to break. Side note, I don't think we should always break at
=> for instance .map { x => (x, x) } but that is another issue probably.
On Sat, Sep 17, 2016 at 07:03 Ólafur Páll Geirsson notifications@github.com
wrote:

The current select chain rules are pretty ad-hoc at the moment and badly
documented https://olafurpg.github.io/scalafmt/#Selectchains.
Considering select chains are really important in Scala I think they
deserve some rethinking.

They rules currently work like this:

  • never line break before a dot . unless the dot is part of a
    breakable select chain.
  • A select chain becomes breakable at the first select dot . that
    leads an application that has parentheses () or square brackets [].
  • Either all dots in a breakable select chain get a line break or none
    of the dots get a line break.
  • By default, scalafmt puts the entire select chain on a single line
    if the chain can fit on a single line. PR #440
    https://github.com/olafurpg/scalafmt/pull/440 added a flag
    keepSelectChainLineBreaks that changes the default behavior so that
    scalafmt will put line breaks before all dots in a breakable select chain
    if there is a newline before the first dot in the breakable part of
    the select chain.

Some examples.

  • a really long sequence of foo.bar.kaz.man.kazam won't line break
    since there is no application with [] or () to make the chain
    breakable. This might mean that the formatted output exceeds the column
    limit.
  • in lst.head.foo(...).kaz the chain becomes breakable at .foo and not
    .head.
  • the following example never becomes breakable because the .collect
    uses curly braces

[info] val cols = flatSchema.getFields.collect {
[info] case f: PrimitiveType =>
[info] Column(f.getName, typeMap(f, widths))
[info] }.toList

  • keepSelectChainLineBreaks is not triggered on the following example
    because there is no newline before .from().

    val serializedBsonByKey= TypedPipe.from()
    .map{Base64.decodeBase64}
    .groupBy{key}
    .head// scalafmt default outputval serializedBsonByKey =
    TypedPipe.from().map { Base64.decodeBase64 }.groupBy { key }.head// scalafmt output with newline before .from and keepSelectChainLineBreaks enabledval serializedBsonByKey = TypedPipe
    .from()
    .map { Base64.decodeBase64 }
    .groupBy { key }
    .head

Some questions

  1. Should curly braces trigger the start of a breakable select chain
    just like [] and () do? I remember I tried this out and for some cases
    it wasn't so great.
  2. What should the behavior of keepSelectChainLineBreaks actually be?
    Should the option be renamed?
  3. More generally, how do you think select chains should format?

The current rules are derived from looking at quite a lot code and any
changes to the rules will probably require looking at more diffs. I
personally quite like the default behavior except in some cases I think
scalafmt should line break select chain even if the chains can fit on a
single line.

cc/ @johnynek https://github.com/johnynek @melrief
https://github.com/melrief


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/olafurpg/scalafmt/issues/444, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdlxT1-Smqp5gxmu5apwdUNF_hGgXks5qrB1bgaJpZM4J_ph0
.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Sep 26, 2016

Member

Following your feedback, I renamed keepSelectChainLineBreaks into optIn.breakChainOnFirstMethodDot. The original name was chosen in a rush during a hackathon.

you only add new lines (never remove them) and you only add new lines in order to satisfy max line width restrictions.

I'm happy to help someone contribute a flag with this behavior. I personally prefer scalafmt to break the whole chain if can't fit on a single line.

Side note, I don't think we should always break at => for instance .map { x => (x, x) } but that is another issue probably.

This sounds related to #357, please pitch in there to bump up the priority.

Member

olafurpg commented Sep 26, 2016

Following your feedback, I renamed keepSelectChainLineBreaks into optIn.breakChainOnFirstMethodDot. The original name was chosen in a rush during a hackathon.

you only add new lines (never remove them) and you only add new lines in order to satisfy max line width restrictions.

I'm happy to help someone contribute a flag with this behavior. I personally prefer scalafmt to break the whole chain if can't fit on a single line.

Side note, I don't think we should always break at => for instance .map { x => (x, x) } but that is another issue probably.

This sounds related to #357, please pitch in there to bump up the priority.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 20, 2016

Member

Here is a diff from including curly braces as part of select chains: https://github.com/olafurpg/scala-repos/pull/12/files

In general, I like the new changes. For example, here is one of my favorites

-    Repositories.filter { t =>
-      (t.originUserName === userName.bind) &&
-      (t.originRepositoryName === repositoryName.bind)
-    }.sortBy(_.userName asc).map(t => t.userName -> t.repositoryName).list
+    Repositories
+      .filter { t =>
+        (t.originUserName === userName.bind) &&
+        (t.originRepositoryName === repositoryName.bind)
+      }
+      .sortBy(_.userName asc)
+      .map(t => t.userName -> t.repositoryName)
+      .list

What I like most about it is how consistent it looks like. I am OK with the extra vertical space. I admit it the floating curly braces may seem weird sometimes

  }
  .map {
    ...
  }
  .bar {

On the other hand, the aligned dots . do make it clear what steps are part of the "pipeline". I also like the fact that it's easier to remove one step from the pipeline by deleting an entire line (dd for vim folks like myself).

I'm tempted to include this change in the default style, with a flag to opt-out.

cc/ @rcavalcanti

Member

olafurpg commented Oct 20, 2016

Here is a diff from including curly braces as part of select chains: https://github.com/olafurpg/scala-repos/pull/12/files

In general, I like the new changes. For example, here is one of my favorites

-    Repositories.filter { t =>
-      (t.originUserName === userName.bind) &&
-      (t.originRepositoryName === repositoryName.bind)
-    }.sortBy(_.userName asc).map(t => t.userName -> t.repositoryName).list
+    Repositories
+      .filter { t =>
+        (t.originUserName === userName.bind) &&
+        (t.originRepositoryName === repositoryName.bind)
+      }
+      .sortBy(_.userName asc)
+      .map(t => t.userName -> t.repositoryName)
+      .list

What I like most about it is how consistent it looks like. I am OK with the extra vertical space. I admit it the floating curly braces may seem weird sometimes

  }
  .map {
    ...
  }
  .bar {

On the other hand, the aligned dots . do make it clear what steps are part of the "pipeline". I also like the fact that it's easier to remove one step from the pipeline by deleting an entire line (dd for vim folks like myself).

I'm tempted to include this change in the default style, with a flag to opt-out.

cc/ @rcavalcanti

@johnynek

This comment has been minimized.

Show comment
Hide comment
@johnynek

johnynek Oct 21, 2016

Contributor

This looks nice to me! Is that from the latest release? I'm sorry to say I got buried on a project and have not updated lately (also we have a lot of engineers using the repo with scalafmt linting on so I want to minimize churn of those rules). This would help us since we have a lot of select chains in scalding code.

Contributor

johnynek commented Oct 21, 2016

This looks nice to me! Is that from the latest release? I'm sorry to say I got buried on a project and have not updated lately (also we have a lot of engineers using the repo with scalafmt linting on so I want to minimize churn of those rules). This would help us since we have a lot of select chains in scalding code.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 21, 2016

Member

That diff is from a PR, #532. The feedback on Gitter was positive, too. I will keep this behavior opt-in for 4.x and flip the switch in 0.5.0.

Any suggestion on a name for the new flag? Currently it's includeCurlyBraceInSelectChains

I think the default behavior for keepSelectChainLineBreaks should be that you only add new lines (never remove them) and you only add new lines in order to satisfy max line width restrictions.

The combination of

optIn.breakChainOnFirstMethodDot = true
includeCurlyBraceInSelectChains = true

should roughly achieve that goal. If not, I'm happy to iterate further.

Member

olafurpg commented Oct 21, 2016

That diff is from a PR, #532. The feedback on Gitter was positive, too. I will keep this behavior opt-in for 4.x and flip the switch in 0.5.0.

Any suggestion on a name for the new flag? Currently it's includeCurlyBraceInSelectChains

I think the default behavior for keepSelectChainLineBreaks should be that you only add new lines (never remove them) and you only add new lines in order to satisfy max line width restrictions.

The combination of

optIn.breakChainOnFirstMethodDot = true
includeCurlyBraceInSelectChains = true

should roughly achieve that goal. If not, I'm happy to iterate further.

@olafurpg olafurpg added this to the 0.5 milestone Oct 23, 2016

olafurpg added a commit that referenced this issue Dec 17, 2016

Write changelog for 0.5
Changed some settings in the default style:
- includeCurlyBraceInSelectChains, fixes #444
- breakChainOnFirstMethodDot

olafurpg added a commit that referenced this issue Dec 17, 2016

Write changelog for 0.5
Changed some settings in the default style:
- includeCurlyBraceInSelectChains, fixes #444
- breakChainOnFirstMethodDot

olafurpg added a commit that referenced this issue Dec 17, 2016

Prepare for 0.5 release.
Changed some settings in the default style:
- includeCurlyBraceInSelectChains, fixes #444
- breakChainOnFirstMethodDot

Wrote changelog to docs.

olafurpg added a commit that referenced this issue Dec 17, 2016

Prepare for 0.5 release.
Changed some settings in the default style:
- includeCurlyBraceInSelectChains, fixes #444
- breakChainOnFirstMethodDot

Wrote changelog to docs.

@olafurpg olafurpg closed this in #613 Dec 18, 2016

olafurpg added a commit that referenced this issue Dec 18, 2016

Prepare for 0.5 release. (#613)
Changed some settings in the default style:
- includeCurlyBraceInSelectChains, fixes #444
- breakChainOnFirstMethodDot

Wrote changelog to docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment