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

Add verticalMultilineAtDefinitionSiteArityThreshold setting #1143

Merged
merged 8 commits into from
Apr 22, 2018
39 changes: 39 additions & 0 deletions readme/Configuration.scalatex
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,45 @@
yield (a, b)
}

@sect{verticalMultilineAtDefinitionSite}
Default: @b{false}

@p
If true, reformat multi-line function definitions in the following way
@fullWidthDemo(verticalAlign)
object a {
def format(code: String, age: Int)(implicit ev: Parser, c: Context): String
}
@p
All parameters are on their own line indented by four (4), separation between
parameter groups are indented by two (2). ReturnType is on its own line at
then end. This will only trigger if the function would go over
maxColumn. If a multi-line function can fit in a single line, it will
make it so. Note that this setting ignores continuation.defnSite,
binPack.unsafeDefnSite, and align.openParenDefnSite.

@p
To enable this add it to the config like this @config{verticalMultilineAtDefinitionSite = true}.

@sect{verticalMultilineAtDefinitionSiteArityThreshold}
Default: @b{100}

@p
If set, this will trigger a vertical multi-line formatting as described above even though the
definition falls below the maxColumn width.

@p
To enable a rewrite rule, add it to the config like this @config{verticalMultilineAtDefinitionSiteArityThreshold = 2}.

@fullWidthDemo(arityThreshold)
case class Foo(x: String)
case class Bar(x: String, y: String)
case class VeryLongNames(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: String)
object A {
def foo(x: String, y: String)
def hello(how: String)(are: String)(you: String) = how + are + you
}

@sect{project}
@p
Configure which source files should be formatted in this project.
Expand Down
16 changes: 16 additions & 0 deletions readme/src/main/scala/org/scalafmt/readme/Readme.scala
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ object Readme {
sideBySide(code, formatted)
}

def fullWidthDemo(style: ScalafmtConfig)(code: String) = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job figuring out the fancy website generation 😄

val formatted = Scalafmt.format(code, style).get
pairs(List(code, formatted).map(x => hl.scala(x)): _*)
}

def demoStyle(style: ScalafmtConfig)(code: String) = {
val formatted =
Scalafmt.format(code, style.copy(runner = ScalafmtRunner.sbt)).get
Expand Down Expand Up @@ -165,6 +170,17 @@ object Readme {
rules = Seq(PreferCurlyFors)
))

val verticalAlign = ScalafmtConfig.default.copy(
maxColumn = 60,
verticalMultilineAtDefinitionSite = true
)

val arityThreshold =
ScalafmtConfig.default.copy(
verticalMultilineAtDefinitionSite = true,
verticalMultilineAtDefinitionSiteArityThreshold = 2
)

def fmt(style: ScalafmtConfig)(code: String): TypedTag[String] =
example(code, style)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,15 @@ import org.scalafmt.util.ValidationOps
* )
* }}}
*
* All parameters are on their on line indented by four (4), seperation between
* parament groups are indented by two (2). ReturnType is on its own line at
* then end. This will only trigger if the function would go over
* [[maxColumn]]. If a multi-line funcion can fit in a single line, it will
* All parameters are on their own line indented by four (4), separation between
* parameter groups are indented by two (2). ReturnType is on its own line at
* the end. This will only be triggered if the function would go over
* [[maxColumn]]. If a multi-line function can fit in a single line, it will
* make it so. Note that this setting ignores continuation.defnSite,
* [[binPack.unsafeDefnSite]], and [[align.openParenDefnSite]].
* @param verticalMultilineAtDefinitionSiteArityThreshold If set, this will trigger a vertical multi-line formatting as
* described above even though the definition falls below the
* [[maxColumn]] width.
*/
@DeriveConfDecoder
case class ScalafmtConfig(
Expand Down Expand Up @@ -142,6 +145,7 @@ case class ScalafmtConfig(
danglingParentheses: Boolean = false,
poorMansTrailingCommasInConfigStyle: Boolean = false,
verticalMultilineAtDefinitionSite: Boolean = false,
verticalMultilineAtDefinitionSiteArityThreshold: Int = 100,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wanted to make this an Option but there isn't a ConfDecoder[Option[A]] defined. Does it make sense to make it an option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why I never implemented a decoder for Option[T], there was some reason. I agree dummy Int values are not great, but it's consistent with some other settings so it's fine here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll leave this as it for now then 👍

onTestFailure: String = "",
encoding: Codec = "UTF-8",
@Recurse project: ProjectFiles = ProjectFiles()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,19 @@ import scala.annotation.tailrec
import scala.collection.mutable
import scala.meta.Case
import scala.meta.Ctor
import scala.meta.Decl
import scala.meta.Defn
import scala.meta.Import
import scala.meta.Name
import scala.meta.Pat
import scala.meta.Pkg
import scala.meta.Template
import scala.meta.Term
import scala.meta.Term.Apply
import scala.meta.Tree
import scala.meta.Type
import scala.meta.prettyprinters.Structure
import scala.meta.tokens.Token
import scala.meta.tokens.Token._
import scala.meta.tokens.Tokens
import org.scalafmt.Error.CaseMissingArrow
import org.scalafmt.config.ScalafmtConfig
import org.scalafmt.internal.ExpiresOn.Left
Expand All @@ -31,7 +30,6 @@ import org.scalafmt.util.TreeOps
import org.scalafmt.util.Whitespace
import org.scalafmt.util.Modifier
import org.scalafmt.util.RightParenOrBracket
import org.scalameta.logger

/**
* Helper functions for generating splits/policies for a given tree.
Expand Down Expand Up @@ -823,8 +821,19 @@ class FormatOps(val tree: Tree, val initStyle: ScalafmtConfig) {
if (isBracket) close // If we can fit the type params, make it so
else lastParen // If we can fit all in one block, make it so

val maxArity = valueParamsOwner match {
case d: Decl.Def if d.paramss.nonEmpty => d.paramss.map(_.size).max
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Init came in scalameta v2 but scalafmt is still on v1,

Seems in v1 Init is nested Term.Apply

@ q"@a(1)(2) def foo = 1"
res1: Defn.Def = Defn.Def(List(Mod.Annot(Term.Apply(Term.Apply(Ctor.Ref.Name("a"), List(Lit.Int(1))), List(Lit.Int(2))))), Term.Name("foo"), List(), List(), None, Lit.Int(1))

It's fine to skip those as they only appear in curried annotations, which is quite rare.

case d: Defn.Def if d.paramss.nonEmpty => d.paramss.map(_.size).max
case m: Defn.Macro if m.paramss.nonEmpty => m.paramss.map(_.size).max
case c: Ctor.Primary if c.paramss.nonEmpty => c.paramss.map(_.size).max
case c: Ctor.Secondary if c.paramss.nonEmpty => c.paramss.map(_.size).max
case _ => 0
}

val aboveArityThreshold = maxArity >= style.verticalMultilineAtDefinitionSiteArityThreshold

Seq(
Split(NoSplit, 0)
Split(NoSplit, 0, ignoreIf = !isBracket && aboveArityThreshold)
.withPolicy(SingleLineBlock(singleLineExpire)),
Split(Newline, 1) // Otherwise split vertically
.withIndent(firstIndent, close, Right)
Expand Down
27 changes: 27 additions & 0 deletions scalafmt-tests/src/test/resources/test/arityThreshold.source
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
maxColumn = 80
verticalMultilineAtDefinitionSite = true
verticalMultilineAtDefinitionSiteArityThreshold = 2
danglingParentheses = true
continuationIndent.defnSite = 2
<<< Verify that verticalMultilineAtDefinitionSiteArityThreshold works as expected
case class Foo(x: String)
case class Bar(x: String, y: String)
case class VeryLongNames(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: String)
object A {
def foo(x: String, y: String)
def hello(how: String)(are: String)(you: String) = how + are + you
}
>>>
case class Foo(x: String)
case class Bar(
x: String,
y: String)
case class VeryLongNames(
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: String)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangling parentheses is explicitly ignored for Ctor.Primary, Defn.Class, and Trait. I'd prefer to have them dangling in these cases but that seems like a separate issue :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olafurpg Do you have any thoughts in this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this a peculiarity of verticalMultilineAtDefinitionSite? If so I have no opinions, but @pjrt might!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mads-hartmann Yeah, the feature ignores dangling (and most other options). It is one of those features that don't make sense if it listens to every other scalafmt option. It is a holistic style, instead of a piece-meal one (like dangling or align).

As for why, well it looks ugly if you allow dangling. So it only dangles when the class extends something.

object A {
def foo(
x: String,
y: String
)
def hello(how: String)(are: String)(you: String) = how + are + you
}