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

Improved alignParameters handling should be opt-in #206

Open
jkinkead opened this issue Mar 1, 2016 · 15 comments
Open

Improved alignParameters handling should be opt-in #206

jkinkead opened this issue Mar 1, 2016 · 15 comments
Assignees
Milestone

Comments

@jkinkead
Copy link
Collaborator

jkinkead commented Mar 1, 2016

alignParameters handling was expanded in the daniel-trinh fork to include aligning equals signs and default values.

This should possibly be hidden behind an option per #142 .

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 1, 2016

Should this be milestone 1.0, @sschaef / @fommil ? It's an improvement to an existing opt-in feature, but I notice that @jrudolph disabled alignParameters in akka/akka#19936 , probably for this reason.

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 1, 2016

To visualize what I'm talking about, here's an example from akka:

diff --git a/akka-actor/src/main/scala/akka/routing/ConsistentHashing.scala b/akka-actor/src/main/scala/akka/routing/ConsistentHashing.scala
index 1139fc1..e0ba2b1 100644
--- a/akka-actor/src/main/scala/akka/routing/ConsistentHashing.scala
+++ b/akka-actor/src/main/scala/akka/routing/ConsistentHashing.scala
@@ -135,9 +135,9 @@ object ConsistentHashingRoutingLogic {
  */
 @SerialVersionUID(1L)
 final case class ConsistentHashingRoutingLogic(
-  system: ActorSystem,
-  virtualNodesFactor: Int = 0,
-  hashMapping: ConsistentHashingRouter.ConsistentHashMapping = ConsistentHashingRouter.emptyConsistentHashMapping)
+  system:             ActorSystem,
+  virtualNodesFactor: Int                                           = 0,
+  hashMapping:        ConsistentHashingRouter.ConsistentHashMapping = ConsistentHashingRouter.emptyConsistentHashMapping)
   extends RoutingLogic {

   import ConsistentHashingRouter._

Note that the types are aligned as are the default values. This is a new addition to the alignParameters setting - previously, it left-justified all arguments, but left the types & defaults alone.

@fommil
Copy link
Contributor

fommil commented Mar 1, 2016

I really like alignment but haven't used it yet.

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 1, 2016

It's more that you can no longer opt-in to formatting like:

// Int is not aligned with String.
def foo(arg1: Int,
        arg2HasALongName: String): Unit

This is the kind of alignment that v0.1.4 does. Akka disabled alignParameters in the PR I referenced, likely because of this issue.

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 2, 2016

Attached is the full Akka diff with all of the new settings from PR #205 . Note that virtually all of these are either arrow renaming changes or the new align parameters setting. FYI @jrudolph if you're interested in comparing with your PR.

akkadiff.txt

@fommil
Copy link
Contributor

fommil commented Mar 2, 2016

😄 wow, we're actually getting near 1.0

@jrudolph
Copy link
Contributor

jrudolph commented Mar 2, 2016

I tried your branch with .setPreference(FirstParameterOnNewline, Preserve) but it helps only a little because of these kind of changes:

   def validate[T1, T2](value1: RequestVal[T1],
-                       value2: RequestVal[T2],
-                       check: Function2[T1, T2, JBoolean],
-                       errorMsg: String,
-                       innerRoute: Route, moreInnerRoutes: Route*): Route =
+    value2: RequestVal[T2],
+    check: Function2[T1, T2, JBoolean],
+    errorMsg: String,
+    innerRoute: Route, moreInnerRoutes: Route*): Route =

So, the first line is now preserved but the indentation of the others still changes.

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 2, 2016

@jrudolph - It looks like you left the .setPreference(AlignParameters, false) change in. Try setting that back to true. :)

@jrudolph
Copy link
Contributor

jrudolph commented Mar 2, 2016

Setting it to true does more than we need:

-  def validate[T1, T2](value1: RequestVal[T1],
-                       value2: RequestVal[T2],
-                       check: Function2[T1, T2, JBoolean],
-                       errorMsg: String,
+  def validate[T1, T2](value1:     RequestVal[T1],
+                       value2:     RequestVal[T2],
+                       check:      Function2[T1, T2, JBoolean],
+                       errorMsg:   String,

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 2, 2016

Precisely. That's what this bug intends to address: The additional alignment of the types & default values in method signatures should be behind a preference, instead of the default behavior of alignParameters.

So, it sounds like this should be milestone 1.0 - @fommil / @sschaef ? I'd tag it myself, but I don't have permissions. :)

@fommil fommil added this to the 1.0 milestone Mar 2, 2016
@fommil
Copy link
Contributor

fommil commented Mar 2, 2016

we should probably get you added

@jkinkead
Copy link
Collaborator Author

jkinkead commented Mar 2, 2016

FYI https://github.com/jkinkead/scalariform/tree/align_parameter_types_tmp has a fix. It's very simple, but it's dependent on #205 .

@jrudolph
Copy link
Contributor

jrudolph commented Mar 4, 2016

@jkinkead thanks, makes sense. If I get to it I'll try your branch.

@jkinkead jkinkead self-assigned this Mar 4, 2016
@jrudolph
Copy link
Contributor

Cool, your new switch works nicely. What's now left over for Akka is

@jkinkead
Copy link
Collaborator Author

Awesome, that list aligns with my observations as well. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants