Implement #863: vertical-multiline for type params #864

pjrt · 2017-03-29T14:45:40Z

Implements enhancement #863 by extending verticalMultilineAtDefinitionSite
to handle type parameters as well as data parameters.

The type parameter group behaves slightly different than the data
parameters. While data parameters are an all-or-nothing (either it all
fits in one line or it goes vertical), type parameters are allowed to go
in one line if they can fit regardless of whether the data parameters
fit too.

This PR does NOT fix #862. I plan on working on that one next but I found implementing this "fix" in vertical-multiline to be simpler. With the knowledge I gained here I can hopefully fix #862 now.

Implements enhancement scalameta#863 by extending verticalMultilineAtDefinitionSite to handle type parameters as well as data parameters. The type parameter group behaves slightly different than the data parameters. While data parameters are an all-or-nothing (either it all fits in one line or it goes vertical), type parameters are allowed to go in one line if they can fit regardless of whether the data parameters fit too.

olafurpg

I have to run out right now so I will have to review later.

olafurpg · 2017-03-29T15:02:48Z

core/src/main/scala/org/scalafmt/util/TreeOps.scala

+    case _: Decl.Def | _: Defn.Def | _: Defn.Macro | _: Defn.Class |
+        _: Defn.Trait | _: Ctor.Secondary =>
+      true
+    case x: Ctor.Primary if x.parent.exists(_.isInstanceOf[Defn.Class]) =>


This seems like a reasonable workaround for the owners issue.

pjrt · 2017-03-29T15:06:48Z

core/src/main/scala/org/scalafmt/util/TreeOps.scala

+   *
+   * For classes this includes primary and secondary Ctors.
+   */
+  def isClassOrDef(tree: Tree): Boolean = tree match {


I created this because I wasn't aware that isDefnSite includes things like type applications.

I'm not sure whether that's correct (type applications are definitions?) but I opted for a more explicit function in order to not break old functionality (and since I might not have complete understanding on what a defn actually means).

Type applications should probably not be included in isDefnSite, I opened #867 to remind myself to investigate that and fix isDefnSite. For now, I think it's OK for you to create your own utility for vertical multiline.

isClassOrDefn is not so accurate here since this also includes Defn.Trait. A more appropriate name could be isDefnSiteWithParams, since it excludes Defn.Val which is a definition but has no parameters. It's up to you what name you prefer.

pjrt · 2017-03-29T15:18:08Z

core/src/main/scala/org/scalafmt/internal/Router.scala

+        val dataParamsOwner = owners(lastParen)
+        @inline def ownerCheck(rp: Token): Boolean = {
+          val owner = owners(rp)
+          owner == leftOwner || owner == dataParamsOwner


The way this works is:

When dealing with defs, then the owners(rp) will always equals to the leftOwner, for both type params and data params

When dealing with classes though, this isn't the case. For classes owners(rp) will only equal leftOwners for type params. This means the second test will kick in for data params (who share the same owner as the lastParen).

I could compare against for both the owner(rp) and the owner(rp).parent. That may work too.

Well that didn't work (messes with local things like def f(parser: Parser[R])). It might work if I put more restrictions on it but I feel my current restriction is pretty solid (no relative checks. We only have one lastParen).

Keeping as is.

pjrt · 2017-03-29T17:48:53Z

This style currently breaks if we have a modified Ctor.

case class SomeClass[A] private (name: String)

Also fix a bug in `findFirst`

pjrt · 2017-03-29T21:33:41Z

core/src/main/scala/org/scalafmt/internal/Router.scala

+        val isClassTrait =
+          leftOwner.is[meta.Ctor.Primary] ||
+          leftOwner.is[meta.Defn.Class] ||
+          leftOwner.is[meta.Defn.Trait]


Since I have the holistic information about the defn (ie: I can ask things like "does this have any data params? Any type params?") I feel like this can all be cleaner (at the cost of, maybe, some code duplication).

Better defined the concepts of `shouldNotDangle` and `mixedParam`. Removed unnecessary checks when catching traits.

olafurpg

Wow! I'm really happy to see this PR, it demonstrates a good understanding of Policies and Splits.

A few comments, otherwise great job 👍

olafurpg · 2017-03-30T08:17:50Z

core/src/main/scala/org/scalafmt/internal/FormatOps.scala

+    def unapply(tok: Token): Boolean = is(tok)
+  }
+
+  object LeftParenOrBracket {


Can we move this to https://github.com/scalameta/scalafmt/blob/235909ffdcbefe18457878a8c95c6c93ff288ed9/core/src/main/scala/org/scalafmt/util/TokenClasses.scala ?

olafurpg · 2017-03-30T08:18:02Z

core/src/main/scala/org/scalafmt/internal/FormatOps.scala

+    def unapply(tok: Token): Boolean = is(tok)
+  }
+
+  object RightParenOrBracket {


Same, TokenClasses.

olafurpg · 2017-03-30T08:23:11Z

core/src/main/scala/org/scalafmt/internal/FormatOps.scala

@@ -108,6 +108,23 @@ class FormatOps(val tree: Tree, val initStyle: ScalafmtConfig) {
    (packages.result(), imports.result(), arguments.toMap, optional)
  }

+  object CtorModifier {


Can we move this to https://github.com/scalameta/scalafmt/blob/master/core/src/main/scala/org/scalafmt/util/TreeClasses.scala ?

The @classifier macro annotation creates a Classifiable type-class instance, which makes it possible to use the Token/Tree.is[T] syntax. You can probably avoid the macro annotation by writing some boilerplate yourself, but I haven't looked at the expanded code. It's up to yourself.

The (undocumented) general rule where I keep utilities is

FormatOps: anything that requires access to the fields in FormatOps, which is custom for each source file.

Token/TreeOps: anything that is not require fields in FormatOps, agnostic to the source file being formatted.

Token/TreeClasses: classifiers like this.

I should probably add this to the contributing docs.

olafurpg · 2017-03-30T08:24:38Z

core/src/main/scala/org/scalafmt/internal/FormatOps.scala

@@ -165,7 +182,7 @@ class FormatOps(val tree: Tree, val initStyle: ScalafmtConfig) {
  @tailrec
  final def findFirst(start: FormatToken, end: Token)(
      f: FormatToken => Boolean): Option[FormatToken] = {
-    if (start.left.start < end.start) None


Whoah, good catch! Was this method used somewhere before? Logic bugs like this tend to pop up in dead code.

It wasn't used anywhere.

olafurpg · 2017-03-30T08:24:55Z

core/src/main/scala/org/scalafmt/internal/Policy.scala

@@ -16,6 +16,18 @@ case class Policy(
    noDequeue: Boolean = false,
    isSingleLine: Boolean = false)(implicit val line: sourcecode.Line) {

+  def merge(other: PartialFunction[Decision, Decision], newExpire: Int): Policy =


Nice 👍 I've written this too many times.

olafurpg · 2017-03-30T08:38:49Z

core/src/test/resources/test/VerticalMultiline.stat

+  ] protected (
+    value: Long,
+    name: String
+  )(implicit ex: Executor)


So I understand correctly, will the last closing parens never dangle? If so, why?

There seems to be missing a test case where the last param list of a class has >1 parameters.

It should not dangle because it creates an ugly result when combined with extends and with (unless we do some manipulation with those, but then it would need to ignore other settings and make the overall style more complex). We do dangle for defs though.

Aha, makes sense. I just remembered now that IntelliJ style does the same, exclude dangling parentheses in defn site (although both def and class)

olafurpg · 2017-03-30T08:42:11Z

core/src/main/scala/org/scalafmt/util/TreeOps.scala

+  }
+
+  /**
+   * Whether [[tree]] is a defintion site


I try structure docstrings like this:

"Returns X if Y(, else Z)?"

olafurpg · 2017-03-30T08:47:04Z

core/src/main/scala/org/scalafmt/util/TreeOps.scala

+   *
+   * For classes this includes primary and secondary Ctors.
+   */
+  def isClassOrDef(tree: Tree): Boolean = tree match {


Type applications should probably not be included in isDefnSite, I opened #867 to remind myself to investigate that and fix isDefnSite. For now, I think it's OK for you to create your own utility for vertical multiline.

isClassOrDefn is not so accurate here since this also includes Defn.Trait. A more appropriate name could be isDefnSiteWithParams, since it excludes Defn.Val which is a definition but has no parameters. It's up to you what name you prefer.

olafurpg · 2017-03-30T08:47:27Z

core/src/main/scala/org/scalafmt/util/TreeOps.scala

@@ -229,6 +229,25 @@ object TreeOps {
    case _ => None
  }

+  /**
+   * Whether the [[tree]] is a class, trait or def


See comment below.

olafurpg · 2017-03-30T08:50:01Z

core/src/main/scala/org/scalafmt/internal/Router.scala


        val firstIndent =
          if (r.is[RightParen]) // An empty param group
            indentSep
          else
            indentParam

+        val singleLineSplit =


Suggestion:

val singleLineExpire = if (isBracket) close // comment else lastParen // comment val singleLineSplit = Split(NoSplit, 0)... ``

This should help with the ever-growing compilation times.

olafurpg

LGTM 👍 Merging once CI is green.

I'm working on the release infrastructure now. Hoping to get a setup so that a release is published to bintray on merge into master and stable releases are published to sonatype on tag push.

…a#864) * Implement scalameta#863: vertical-multiline for type params Implements enhancement scalameta#863 by extending verticalMultilineAtDefinitionSite to handle type parameters as well as data parameters. The type parameter group behaves slightly different than the data parameters. While data parameters are an all-or-nothing (either it all fits in one line or it goes vertical), type parameters are allowed to go in one line if they can fit regardless of whether the data parameters fit too. * Add trait tests * Fix case: Class with just type params Also fix a bug in `findFirst` * Fix trait inconsistency * Simplify verticalMultiline Better defined the concepts of `shouldNotDangle` and `mixedParam`. Removed unnecessary checks when catching traits. * Fixed some docs and move classifiers to their respective places * Extract verticalMultiline impl from Router This should help with the ever-growing compilation times. * Ran Scalafmt * More doc and style changes

pjrt requested a review from olafurpg March 29, 2017 14:45

olafurpg reviewed Mar 29, 2017

View reviewed changes

pjrt commented Mar 29, 2017

View reviewed changes

Add trait tests

4da6318

pjrt added 2 commits March 29, 2017 15:41

Fix case: Class with just type params

03b39a4

Also fix a bug in `findFirst`

Fix trait inconsistency

2c17cdd

pjrt commented Mar 29, 2017

View reviewed changes

Simplify verticalMultiline

3005795

Better defined the concepts of `shouldNotDangle` and `mixedParam`. Removed unnecessary checks when catching traits.

olafurpg reviewed Mar 30, 2017

View reviewed changes

pjrt added 4 commits March 30, 2017 09:26

Fixed some docs and move classifiers to their respective places

3dc3b40

Extract verticalMultiline impl from Router

9d47b3f

This should help with the ever-growing compilation times.

Ran Scalafmt

dc1bc98

More doc and style changes

44275c5

olafurpg approved these changes Mar 30, 2017

View reviewed changes

olafurpg merged commit 63bd729 into scalameta:master Mar 30, 2017

pjrt mentioned this pull request Mar 30, 2017

Scalafmt does not indent type members when there are more than one #862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #863: vertical-multiline for type params #864

Implement #863: vertical-multiline for type params #864

pjrt commented Mar 29, 2017

olafurpg left a comment

olafurpg Mar 29, 2017

pjrt Mar 29, 2017

olafurpg Mar 30, 2017

pjrt Mar 29, 2017

pjrt Mar 29, 2017

pjrt Mar 29, 2017

pjrt commented Mar 29, 2017

pjrt Mar 29, 2017

olafurpg left a comment

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

pjrt Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

pjrt Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg Mar 30, 2017

olafurpg left a comment

Implement #863: vertical-multiline for type params #864

Implement #863: vertical-multiline for type params #864

Conversation

pjrt commented Mar 29, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjrt commented Mar 29, 2017

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment