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

Missing linebreak for parameterless method definition #600

Closed
aeons opened this Issue Nov 28, 2016 · 6 comments

Comments

2 participants
@aeons
Contributor

aeons commented Nov 28, 2016

It seems that parameterless method definitions do not get formatted correctly, if they are one character too long.

The following example gets formatted to the expected format when given a maxColumn of 31, but at 32, no linebreaks are inserted. The comment is 32 characters.

Version: 0.4.10
Configuration:

style = defaultWithAlign
maxColumn = 32

Input

object Test {
//-----------------------------|
  def test: Either[String, Int] = Right(0)
}

Scalafmt

object Test {
//-----------------------------|
  def test: Either[String, Int] =
    Right(0)
}

Expected

object Test {
//-----------------------------|
  def test: Either[String,
                   Int] =
    Right(0)
}
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 28, 2016

Member

Interesting! Thanks for reporting. Could it be that your example is off-by-one? If I lower the max column to 31 I get

// scalafmt: { maxColumn = 31, style = defaultWithAlign }
object Test {
  //-----------------------------|
  def test: Either[String,
                   Int] =
    Right(0)
}
Member

olafurpg commented Nov 28, 2016

Interesting! Thanks for reporting. Could it be that your example is off-by-one? If I lower the max column to 31 I get

// scalafmt: { maxColumn = 31, style = defaultWithAlign }
object Test {
  //-----------------------------|
  def test: Either[String,
                   Int] =
    Right(0)
}
@aeons

This comment has been minimized.

Show comment
Hide comment
@aeons

aeons Nov 28, 2016

Contributor

The example might be, but I see this occasionally with longer lines, where scalastyle complains about too long lines, and scalafmt insists on keeping the = as the x+1 character.

Contributor

aeons commented Nov 28, 2016

The example might be, but I see this occasionally with longer lines, where scalastyle complains about too long lines, and scalafmt insists on keeping the = as the x+1 character.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 28, 2016

Member

Scalafmt may prefer to keep the = on the x+1 character instead of breaking the line, since breaking the line can hurt readability. Have you tried configuring scalastyle to a slightly higher max column than scalafmt.

Member

olafurpg commented Nov 28, 2016

Scalafmt may prefer to keep the = on the x+1 character instead of breaking the line, since breaking the line can hurt readability. Have you tried configuring scalastyle to a slightly higher max column than scalafmt.

@aeons

This comment has been minimized.

Show comment
Hide comment
@aeons

aeons Nov 28, 2016

Contributor

Yeah, I guess that could work, however this is the only case where I have seen that it fails, and it is only that one character that changes whether it will break the line or not. To use the specific example that prompted this:

private[uploadtool] def publishPipe: Pipe[Either[UploadToolError, Publishable], Either[UploadToolError, Publishable]] =

That line is 121 characters long and our settings has maxColumn = 120.

If you add one character to the name, you get this, which is what I would have expected from the above as well.

private[uploadtool] def publishPipe1: Pipe[Either[UploadToolError, Publishable],
                                           Either[UploadToolError, Publishable]] =

It smells like an off-by-one error somewhere around handling of =.

Contributor

aeons commented Nov 28, 2016

Yeah, I guess that could work, however this is the only case where I have seen that it fails, and it is only that one character that changes whether it will break the line or not. To use the specific example that prompted this:

private[uploadtool] def publishPipe: Pipe[Either[UploadToolError, Publishable], Either[UploadToolError, Publishable]] =

That line is 121 characters long and our settings has maxColumn = 120.

If you add one character to the name, you get this, which is what I would have expected from the above as well.

private[uploadtool] def publishPipe1: Pipe[Either[UploadToolError, Publishable],
                                           Either[UploadToolError, Publishable]] =

It smells like an off-by-one error somewhere around handling of =.

@aeons

This comment has been minimized.

Show comment
Hide comment
@aeons

aeons Nov 28, 2016

Contributor

Oh yeah, if the method has parameters, it will prefer breaking the line after the opening paren in the parameter list.

Contributor

aeons commented Nov 28, 2016

Oh yeah, if the method has parameters, it will prefer breaking the line after the opening paren in the parameter list.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 28, 2016

Member

It smells like an off-by-one error somewhere around handling of =

That sounds very likely : ) And I am not surprised if it's caused by a minor difference in how methods with and without parentheses are handled.

I am happy to help anyone who's interested to try and fix this issue. However, I don't expect I will look into it myself.

A good place to start investigating this is to read up on this tutorial https://github.com/olafurpg/scalafmt#hacking-on-scalafmt. In Router.scala, keep an eye out for "OptimalToken" or "expire token".

Member

olafurpg commented Nov 28, 2016

It smells like an off-by-one error somewhere around handling of =

That sounds very likely : ) And I am not surprised if it's caused by a minor difference in how methods with and without parentheses are handled.

I am happy to help anyone who's interested to try and fix this issue. However, I don't expect I will look into it myself.

A good place to start investigating this is to read up on this tutorial https://github.com/olafurpg/scalafmt#hacking-on-scalafmt. In Router.scala, keep an eye out for "OptimalToken" or "expire token".

aeons added a commit to aeons/scalafmt that referenced this issue May 31, 2017

aeons added a commit to aeons/scalafmt that referenced this issue May 31, 2017

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