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

Multiple parameters list #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

daniel-trinh
Copy link
Owner

PR from mdr branch:
scala-ide#99

@bambuchaAdm
Copy link

10x for reactivating scalariform

@bambuchaAdm
Copy link

Do you think we should increase limit for parameter from 100 characters ?

@daniel-trinh
Copy link
Owner Author

For the limit, I think it's fine to have it as 1 to 100, since that's the same range of
AlignSingleLineCaseStatements.MaxArrowIndent at the moment.

This feature is a bit tricky to integrate when a parameter clause spans multiple lines:

Currently, enabling this feature would make something like this

def something(
  a: Int, 
  b: String, 
  c: Boolean)(
  d: Int, 
  e: String, 
  f: Boolean)

look like this:

def something(
  a: Int, 
  b: String, 
  c: Boolean)
             (
  d: Int, 
  e: String, 
  f: Boolean)

I think it would make sense to add a check so that all param clauses must be single line only
for this feature -- that way it won't look strange with multi line parameter clauses.

It would also make sense to implement this same behavior for method arguments as well:

  something(1, "", false)(2, "e", true)

would be rewritten to

  something(1, "", false)
           (2, "e", true)

@bambuchaAdm
Copy link

Should I add more integration test ? But what is right behavior of this feature when other are active ?

@bambuchaAdm
Copy link

I plan to create next PR but it can wait until we have consensus on how it should work.

@daniel-trinh
Copy link
Owner Author

@bambuchaAdm Are you still planning on working on this feature? If not, I'll pick it up as the next thing I implement.

More thoughts on implementation details:

I'm thinking that it might be simpler to not include extra configuration options in favor
of detecting whether or not the user wants their parameter clauses on multiple lines:

Examples:

This...

def a(one: Int)(two: String): Boolean

a(1)("2")

stays unchanged:

def a(one: Int)(two: String): Boolean

a(1)("2")

A newline between clauses...

def a(one: Int)
(two: String): Boolean

a(1)
("2")

becomes:

def a(one: Int)
     (two: String): Boolean

a(1)
 ("2")

@bambuchaAdm
Copy link

I'm going to finish this just after implement annotation. feasturue.

At second proposition. I have wrote minimal

object App extends Application {
  def mpl(i: Int)(s: String) = ???

  mpl(5)
     ("Ala")
}

and on compilation in SBT i have

[error] /tmp/test/App.scala:4: missing arguments for method mpl in object App;
[error] follow this method with `_' if you want to treat it as a partially applied function
[error]   mpl(5)
[error]      ^
[warn] /tmp/test/App.scala:5: a pure expression does nothing in statement position; you may be omitting necessary parentheses
[warn]      ("Ala")
[warn]       ^
[warn] one warning found

So we cannot use that syntax on callsite

@daniel-trinh
Copy link
Owner Author

OK, good to know. That simplifies the implementation a bit.

@bambuchaAdm
Copy link

I think is's work. But its need some refactoring. #7 will simplified implementation but i don't use it here.

@betehess
Copy link

betehess commented Oct 4, 2014

What's the status of this PR?

@daniel-trinh
Copy link
Owner Author

@betehess I ran into conflicts trying to integrate this feature without breaking the alignParameter feature. This feature is on hold until those conflicts are fixed

@ktoso
Copy link

ktoso commented Mar 6, 2015

Any chances to revive this one, @bambuchaAdm? Bumped into needing it recently as well..

@bambuchaAdm bambuchaAdm force-pushed the multiple-parameters-list branch from 3383e5a to 19faa4a Compare March 19, 2015 04:29
@bambuchaAdm
Copy link

Initial rework after changes on master. 1 test still fail. I think this should be ready to end of week.

@bambuchaAdm
Copy link

All tests are green. Ready to merge. @daniel-trinh please look on it quickly before conflicts appear.

@daniel-trinh
Copy link
Owner Author

It looks like this feature doesn't play well with the AlignParameters feature. Trying to merge these two functionalities and have them play nicely might be a hairy task. It might be best to simply disallow using the two features together. Thoughts?

Example of conflicts (seems like AlignParameter code overrides the BreakMultipleParameterGroups behavior):

// with preferences FormattingPreferences(Map(BreakMultipleParameterGroups -> true, BreakingThreshold -> 4, AlignParameters -> true)) in version 2.10.4 *** FAILED ***
   Format failure:
    ---- Expected ----
   def f(x: Int,¶
   y: Int)¶
        (y: Int, z: Int): Int = {¶
   }¶
   <<<
    ---- but was-----
   def f(¶
     x: Int,¶
     y: Int¶
   )(¶
     y: Int,¶
     z: Int¶
   ): Int = {¶
   }¶
   <<<
    ---- Original: -----
   def f(¶
     x: Int,¶
     y: Int¶
   )(¶
     y: Int,¶
     z: Int¶
   ): Int = {¶
   }¶
 // with preferences FormattingPreferences(Map(BreakMultipleParameterGroups -> true, BreakingThreshold -> 4, AlignParameters -> true)) in version 2.10.4 *** FAILED ***
   Format failure:
    ---- Expected ----
   def f(x: Int)¶
        (y: Int)¶
        (z: Int): Int = {¶
   }¶
   <<<
    ---- but was-----
   def f(x: Int)(y: Int)(z: Int): Int = {¶
   }¶
   <<<
    ---- Original: -----
   def f(x: Int)¶
        (y: Int)(z: Int): Int = {¶
   }¶

@jawshooah
Copy link

 ---- Expected ----
def f(x: Int,¶
y: Int)¶
     (y: Int, z: Int): Int = {¶
}¶
<<<

Is this really the expected result of AlignParameters and BreakMultipleParameterGroups interacting? Looks pretty ugly to my eyes.

@daniel-trinh
Copy link
Owner Author

It's not -- I actually haven't thought about the best way for the two to interact -- the failed examples are just to show that the behavior of enabling both is probably not going to result in something that the user expects.

@joprice
Copy link

joprice commented Jun 16, 2015

+1

@kiritsuku
Copy link
Collaborator

Hey @bambuchaAdm, scalariform has new maintainers. Are you interested in bringing this PR back to live? You would have to rebase against the master branch of https://github.com/scala-ide/scalariform

@mjaghouri
Copy link

I am also interested in better formatting of curried parameter lists. Anything going to happen on that?

@kiritsuku
Copy link
Collaborator

See the issues on the upstream project https://github.com/scala-ide/scalariform, maybe there were changes in the meantime

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

Successfully merging this pull request may close these issues.

8 participants