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

Merged
merged 9 commits into from Mar 30, 2017

Conversation

Projects
None yet
2 participants
@pjrt
Collaborator

pjrt commented Mar 29, 2017

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.

Implement #863: vertical-multiline for type params
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.

@pjrt pjrt requested a review from olafurpg Mar 29, 2017

@olafurpg

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

+ 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 comment has been minimized.

@olafurpg

olafurpg Mar 29, 2017

Member

This seems like a reasonable workaround for the owners issue.

@olafurpg

olafurpg Mar 29, 2017

Member

This seems like a reasonable workaround for the owners issue.

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

This comment has been minimized.

@pjrt

pjrt Mar 29, 2017

Collaborator

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

@pjrt

pjrt Mar 29, 2017

Collaborator

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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

olafurpg Mar 30, 2017

Member

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.

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

This comment has been minimized.

@pjrt

pjrt Mar 29, 2017

Collaborator

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

pjrt Mar 29, 2017

Collaborator

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

This comment has been minimized.

@pjrt

pjrt Mar 29, 2017

Collaborator

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

@pjrt

pjrt Mar 29, 2017

Collaborator

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

This comment has been minimized.

@pjrt

pjrt Mar 29, 2017

Collaborator

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

pjrt Mar 29, 2017

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@pjrt

pjrt Mar 29, 2017

Collaborator

This style currently breaks if we have a modified Ctor.

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

pjrt commented Mar 29, 2017

This style currently breaks if we have a modified Ctor.

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

pjrt added some commits Mar 29, 2017

Fix case: Class with just type params
Also fix a bug in `findFirst`
+ val isClassTrait =
+ leftOwner.is[meta.Ctor.Primary] ||
+ leftOwner.is[meta.Defn.Class] ||
+ leftOwner.is[meta.Defn.Trait]

This comment has been minimized.

@pjrt

pjrt Mar 29, 2017

Collaborator

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

@pjrt

pjrt Mar 29, 2017

Collaborator

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

Simplify verticalMultiline
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 👍

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

Same, TokenClasses.

@olafurpg

olafurpg Mar 30, 2017

Member

Same, TokenClasses.

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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

olafurpg Mar 30, 2017

Member

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.

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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

@olafurpg

olafurpg Mar 30, 2017

Member

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

This comment has been minimized.

@pjrt

pjrt Mar 30, 2017

Collaborator

It wasn't used anywhere.

@pjrt

pjrt Mar 30, 2017

Collaborator

It wasn't used anywhere.

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

Nice 👍 I've written this too many times.

@olafurpg

olafurpg Mar 30, 2017

Member

Nice 👍 I've written this too many times.

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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.

@olafurpg

olafurpg Mar 30, 2017

Member

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.

This comment has been minimized.

@pjrt

pjrt Mar 30, 2017

Collaborator

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.

@pjrt

pjrt Mar 30, 2017

Collaborator

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.

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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

olafurpg Mar 30, 2017

Member

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

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

I try structure docstrings like this:

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

@olafurpg

olafurpg Mar 30, 2017

Member

I try structure docstrings like this:

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

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

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

olafurpg Mar 30, 2017

Member

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.

@@ -229,6 +229,25 @@ object TreeOps {
case _ => None
}
+ /**
+ * Whether the [[tree]] is a class, trait or def

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

See comment below.

@olafurpg

olafurpg Mar 30, 2017

Member

See comment below.

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

This comment has been minimized.

@olafurpg

olafurpg Mar 30, 2017

Member

Suggestion:

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

olafurpg Mar 30, 2017

Member

Suggestion:

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

pjrt added some commits Mar 30, 2017

Extract verticalMultiline impl from Router
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.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details

pjrt added a commit to pjrt/scalafmt that referenced this pull request May 22, 2017

Implement #863: vertical-multiline for type params (#864)
* Implement #863: vertical-multiline for type params

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.

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment