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

Don't exclude newlines inside blocks for single-args in IntelliJ style #593

Closed
olafurpg opened this Issue Nov 18, 2016 · 10 comments

Comments

Projects
None yet
2 participants
@olafurpg
Member

olafurpg commented Nov 18, 2016

From gitter: https://gitter.im/olafurpg/scalafmt?at=582ec9c6e51081951edf28dc

lihaoyi fluent$ cat .scalafmt.conf
style = IntelliJ
maxColumn = 100
#align.openParenDefnSite = true
lihaoyi fluent$ cat foo.scala
object Bar {
  println(
    for (x <- Nil) yield {
      2
    }
  )
}
lihaoyi fluent$ scalafmt -c .scalafmt.conf  -f foo.scala
object Bar {
  println(for (x <- Nil) yield {
    2
  })
}
@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 18, 2016

It seems to wrap oddly even if there are multiple args passed to the println:

lihaoyi fluent$ cat foo.scala
object Bar {
  println(
    for (x <- Nil) yield {
      2
    },
    234
  )
}
lihaoyi fluent$ scalafmt -c .scalafmt.conf  -f foo.scala
object Bar {
  println(for (x <- Nil) yield {
    2
  }, 234)
}

Ideally I'd want it to remain with each argument on its own line

lihaoyi commented Nov 18, 2016

It seems to wrap oddly even if there are multiple args passed to the println:

lihaoyi fluent$ cat foo.scala
object Bar {
  println(
    for (x <- Nil) yield {
      2
    },
    234
  )
}
lihaoyi fluent$ scalafmt -c .scalafmt.conf  -f foo.scala
object Bar {
  println(for (x <- Nil) yield {
    2
  }, 234)
}

Ideally I'd want it to remain with each argument on its own line

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 18, 2016

A repro without the for:

lihaoyi fluent$ cat foo.scala
object Bar {
  println(
    println{
      2
    },
    234
  )
}
lihaoyi fluent$ scalafmt -c .scalafmt.conf  -f foo.scala
object Bar {
  println(println {
    2
  }, 234)
}

lihaoyi commented Nov 18, 2016

A repro without the for:

lihaoyi fluent$ cat foo.scala
object Bar {
  println(
    println{
      2
    },
    234
  )
}
lihaoyi fluent$ scalafmt -c .scalafmt.conf  -f foo.scala
object Bar {
  println(println {
    2
  }, 234)
}
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

I think I know what's happening, scalafmt treats those examples as "single-line" argument lists because the newlines inside the curly braces are not included. That's at least how the default style works and I don't think it should apply to the intellij style.

Member

olafurpg commented Nov 18, 2016

I think I know what's happening, scalafmt treats those examples as "single-line" argument lists because the newlines inside the curly braces are not included. That's at least how the default style works and I don't think it should apply to the intellij style.

@olafurpg olafurpg changed the title from Preserve config style for single arg Term.ForYield to Don't exclude newlines inside blocks for single-args in IntelliJ style Nov 18, 2016

olafurpg added a commit that referenced this issue Nov 18, 2016

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

Okay, so I don't think we should make this the default behavior in IntelliJ style since this behavior was specifically requested for patterns like this

Props(new Actor {
  context.stop(self)
  def receive = Actor.emptyBehavior
})

#595 adds a new flag newlines.neverInDanglingParenthesesSingleLineArgList, which you could set to true to get your desired output. I admit the name is a bit unwieldy, but I struggled to come up with something clearer. WDYT?

Member

olafurpg commented Nov 18, 2016

Okay, so I don't think we should make this the default behavior in IntelliJ style since this behavior was specifically requested for patterns like this

Props(new Actor {
  context.stop(self)
  def receive = Actor.emptyBehavior
})

#595 adds a new flag newlines.neverInDanglingParenthesesSingleLineArgList, which you could set to true to get your desired output. I admit the name is a bit unwieldy, but I struggled to come up with something clearer. WDYT?

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 18, 2016

A separate flag looks great to me.


An alternative could be to fall back to ignoring newlines as long as there's only a single argument, and not-ignoring them as long as there are multiple arguments. For example,

foo(bar(
  lols
))

Is something that people may want, but

foo(bar(
  lols
), qux)

or

foo(qux, bar(
  lols
))

are both probably things people don't want often. Furthermore, this doesn't seem related at all to whether the thing inside has curlies or parens. In both cases, as long as there's one argument, I've wanted the arg-starts-on-same-line formatting.


A third alternative, could be to accept both cases: if a user put the args on a separate line, leave them there. If the user put the arg on the next line, leave that there too. Not sure how easy this is but as I understand it scalafmt is happy with being sloppy in certain cases

lihaoyi commented Nov 18, 2016

A separate flag looks great to me.


An alternative could be to fall back to ignoring newlines as long as there's only a single argument, and not-ignoring them as long as there are multiple arguments. For example,

foo(bar(
  lols
))

Is something that people may want, but

foo(bar(
  lols
), qux)

or

foo(qux, bar(
  lols
))

are both probably things people don't want often. Furthermore, this doesn't seem related at all to whether the thing inside has curlies or parens. In both cases, as long as there's one argument, I've wanted the arg-starts-on-same-line formatting.


A third alternative, could be to accept both cases: if a user put the args on a separate line, leave them there. If the user put the arg on the next line, leave that there too. Not sure how easy this is but as I understand it scalafmt is happy with being sloppy in certain cases

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 19, 2016

Member

The third alternative is pretty much how the default style works. If you have a newline before the closing parens and after the opening parens, then scalafmt will keep "config style".

Member

olafurpg commented Nov 19, 2016

The third alternative is pretty much how the default style works. If you have a newline before the closing parens and after the opening parens, then scalafmt will keep "config style".

@olafurpg olafurpg closed this in 0598817 Nov 19, 2016

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 20, 2016

@olafurpg do you mean it's how the default style works before or after your fixes? Because if that's really how it works I don't think I actually need another flag; it's just that my experiments showed it always forcing the arg-on-same-line case even when it wasn't wanted

lihaoyi commented Nov 20, 2016

@olafurpg do you mean it's how the default style works before or after your fixes? Because if that's really how it works I don't think I actually need another flag; it's just that my experiments showed it always forcing the arg-on-same-line case even when it wasn't wanted

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 20, 2016

Member

Before the fix, but it would require a minor adjustment to optIn.configStyleArguments. If we can avoid a new flag then I think that would be perfect. Instead of newlines.neverInDanglingParenthesesSingleLineArgList = true you could enable optIn.configStyleArguments = true (which is false in intellij style but true in default style). The difference between the two is only for cases when the argument list is formatted in "config style" but it could fit on a single line.

/* newlines.neverInDanglingParenthesesSingleLineArgList = true */
// before
foo(
  1
)
// after
foo(1)

/*optIn.configStyleArguments = true */
// before
foo(
  1
)
// after
foo(
  1
)
// before (missing newline after open paren)
foo(  1
)
// after
foo(1)
// before (missing newline after closing paren)
foo(
  1)
// after
foo(1)

With optIn.configStyleArguments = true, you have more control over the line breaks.

If this sounds OK, then I'll go ahead and remove newlines.neverInDanglingParenthesesSingleLineArgList and make the necessary adjustments to
optIn.configStyleArguments.

Member

olafurpg commented Nov 20, 2016

Before the fix, but it would require a minor adjustment to optIn.configStyleArguments. If we can avoid a new flag then I think that would be perfect. Instead of newlines.neverInDanglingParenthesesSingleLineArgList = true you could enable optIn.configStyleArguments = true (which is false in intellij style but true in default style). The difference between the two is only for cases when the argument list is formatted in "config style" but it could fit on a single line.

/* newlines.neverInDanglingParenthesesSingleLineArgList = true */
// before
foo(
  1
)
// after
foo(1)

/*optIn.configStyleArguments = true */
// before
foo(
  1
)
// after
foo(
  1
)
// before (missing newline after open paren)
foo(  1
)
// after
foo(1)
// before (missing newline after closing paren)
foo(
  1)
// after
foo(1)

With optIn.configStyleArguments = true, you have more control over the line breaks.

If this sounds OK, then I'll go ahead and remove newlines.neverInDanglingParenthesesSingleLineArgList and make the necessary adjustments to
optIn.configStyleArguments.

@olafurpg olafurpg reopened this Nov 20, 2016

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 21, 2016

Looks good to me. Here are a few more cases I think are worth noting:

/*optIn.configStyleArguments = true */
// single-line remains single-line

// before
foo(1)
// after
foo(1)
// except if it's too long, then it should be "force" wrapped

// before
foo("12345678901234567890123456789012345678901234567890123456789012345678901234567890")
// after
foo(
  "12345678901234567890123456789012345678901234567890123456789012345678901234567890"
)

The second case being basically what's described in https://github.com/olafurpg/scalafmt/issues/599

lihaoyi commented Nov 21, 2016

Looks good to me. Here are a few more cases I think are worth noting:

/*optIn.configStyleArguments = true */
// single-line remains single-line

// before
foo(1)
// after
foo(1)
// except if it's too long, then it should be "force" wrapped

// before
foo("12345678901234567890123456789012345678901234567890123456789012345678901234567890")
// after
foo(
  "12345678901234567890123456789012345678901234567890123456789012345678901234567890"
)

The second case being basically what's described in https://github.com/olafurpg/scalafmt/issues/599

olafurpg added a commit that referenced this issue Dec 17, 2016

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Dec 17, 2016

Member

This turned out to be super easy to accommodate. The settings

style = IntelliJ
optIn.configStyleArguments = true

should match what we have discussed. In a nutshell, if you put a newline after ( and newline before ) then scalafmt will keep it like that, regardless of whether the line can fit in a single line or not.

// scalafmt keeps line breaks this even if it can fit `foo(1)` on a single line
foo(
  1
)
Member

olafurpg commented Dec 17, 2016

This turned out to be super easy to accommodate. The settings

style = IntelliJ
optIn.configStyleArguments = true

should match what we have discussed. In a nutshell, if you put a newline after ( and newline before ) then scalafmt will keep it like that, regardless of whether the line can fit in a single line or not.

// scalafmt keeps line breaks this even if it can fit `foo(1)` on a single line
foo(
  1
)

@olafurpg olafurpg closed this in 4f627dc Dec 17, 2016

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