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鈥檒l 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

Conversation

3 participants
@mads-hartmann
Contributor

mads-hartmann commented Apr 21, 2018

This is a proof of concept for #1142. I'm opening a PR so I can ask questions about the code 馃槃

mads-hartmann added some commits Apr 21, 2018

Add support for setting an arity threshold on multi-line definitions
This is just a proof of concept for now. I'm committing this so I have
some concrete code to discuss during a PR process.
Add a test case for the arity threshold
forgot it in a the last commit.
@@ -142,6 +145,7 @@ case class ScalafmtConfig(
danglingParentheses: Boolean = false,
poorMansTrailingCommasInConfigStyle: Boolean = false,
verticalMultilineAtDefinitionSite: Boolean = false,
verticalMultilineAtDefinitionSiteArityThreshold: Int = 100,

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

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?

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

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

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

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

@@ -823,8 +821,16 @@ 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 arity = valueParamsOwner match {

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

I'm cheating here. I'd prefer to make this decision for each parameter group (in the case of functions) but I couldn't figure out how.

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

Hmm, not sure I follow. Can you elaborate?

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

Hah, my bad. I'm more confused than I though it seems. Given this code I would have assumed that the following would've triggered a reformat:

object A {
  def hello(how: String)(are: String)(you: String) = how + are + you
}

as it would hit case d: Decl.Def and d.paramss.foldLeft(0)(_ + _.size) would return 3.

I extended the test and it does the right thing, I'm just not sure why.

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

If I add Defn.Def to the match case it does indeed end up formatting it as

object A {
  def hello(
    how: String
  )(are: String
  )(you: String
  ) = how + are + you
}

Which isn't quite what I'd want 馃槄

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

I came up with a solution in 4a034ce

x: String,
y: String)
case class VeryLongNames(
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: String)

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

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

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

@olafurpg Do you have any thoughts in this?

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

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

This comment has been minimized.

@pjrt

pjrt Apr 22, 2018

Collaborator

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

@olafurpg

Overall looks good! A few minor comments, we should be able to fit this in before the next release :)

@@ -142,6 +145,7 @@ case class ScalafmtConfig(
danglingParentheses: Boolean = false,
poorMansTrailingCommasInConfigStyle: Boolean = false,
verticalMultilineAtDefinitionSite: Boolean = false,
verticalMultilineAtDefinitionSiteArityThreshold: Int = 100,

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

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

@@ -823,8 +821,16 @@ 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 arity = valueParamsOwner match {
case d: Decl.Def => d.paramss.foldLeft(0)(_ + _.size)

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

Missing Defn.Def, Defn.Macro, Ctor.Secondary and Init

Scalameta trees are quite precise, which is sometimes annoying but other times a blessing

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

Also, should this rule affect call-site? In that case you need to extract from Term.Apply.

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

No IMO it shouldn't affect the call-site

This comment has been minimized.

@mads-hartmann

mads-hartmann Apr 21, 2018

Contributor

I can't find Init anywhere? :)

@@ -823,8 +821,16 @@ 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 arity = valueParamsOwner match {

This comment has been minimized.

@olafurpg

olafurpg Apr 21, 2018

Member

Hmm, not sure I follow. Can you elaborate?

mads-hartmann added some commits Apr 21, 2018

Add missing match cases and change heuristics to use max arity
This makes sure that we only force reformatting if at least one of the
parameter groups exceed the arity threshold
@olafurpg

LGTM 馃憤

I'll let @pjrt take a look since he's the author of verticalMultilineAtDefinitionSite

@@ -96,6 +96,11 @@ object Readme {
sideBySide(code, formatted)
}
def fullWidthDemo(style: ScalafmtConfig)(code: String) = {

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Good job figuring out the fancy website generation 馃槃

@@ -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

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

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.

x: String,
y: String)
case class VeryLongNames(
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: String)

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

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

@mads-hartmann

This comment has been minimized.

Contributor

mads-hartmann commented Apr 22, 2018

@olafurpg Thanks a lot for reviewing the code 馃憤

@pjrt

This comment has been minimized.

Collaborator

pjrt commented Apr 22, 2018

Look great! Good job. Glad to see I'm not the only one using this feature :)

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 22, 2018

Great :shipit:

We'll probably wait with the release until #1140 is in as well.

@olafurpg olafurpg merged commit 33ed2ef into scalameta:master Apr 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mads-hartmann mads-hartmann deleted the mads-hartmann:arity-threshold branch Apr 22, 2018

@olafurpg olafurpg changed the title from Arity threshold to Add verticalMultilineAtDefinitionSiteArityThreshold setting Apr 23, 2018

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