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

Adding Braces when maxline is below a certain limit #2019

Closed
vito-c opened this issue Jun 13, 2020 · 17 comments · Fixed by #2953
Closed

Adding Braces when maxline is below a certain limit #2019

vito-c opened this issue Jun 13, 2020 · 17 comments · Fixed by #2953

Comments

@vito-c
Copy link

vito-c commented Jun 13, 2020

scalafmt.conf:

rewrite.rules = [AvoidInfix, PreferCurlyFors, SortImports, SortModifiers, RedundantBraces]
rewrite.redundantBraces.methodBodies = true
rewrite.redundantBraces.includeUnitMethods = true
rewrite.redundantBraces.maxLines = 100
danglingParentheses.defnSite = true
danglingParentheses.callSite = true

Input code:

  /** Redundant braces removes shouldn't remove braces */
  def bracesStay(): Option[Unit] = {
    Some("thing").map { u1 =>
      Unit
    }
  }

output code:

  /** Redundant braces please add some braces */
  def bracesAdd(): Option[Unit] =
    Some("thing").map { u1 =>
      Unit
    }

That checks out and works as expected but it is impossible to undo the change by changing the maxLine

scalafmt.conf:

rewrite.rules = [AvoidInfix, PreferCurlyFors, SortImports, SortModifiers, RedundantBraces]
rewrite.redundantBraces.methodBodies = true
rewrite.redundantBraces.includeUnitMethods = true
rewrite.redundantBraces.maxLines = 1
danglingParentheses.defnSite = true
danglingParentheses.callSite = true

input code:

  /** Redundant braces please add some braces */
  def bracesAdd(): Option[Unit] =
    Some("thing").map { u1 =>
      Unit
    }

output code:

  /** Redundant braces please add some braces */
  def bracesAdd(): Option[Unit] =
    Some("thing").map { u1 =>
      Unit
    }

expected code:

  /** Redundant braces please add some braces */
  def bracesAdd(): Option[Unit] = {
    Some("thing").map { u1 =>
      Unit
    }
  }
@kitbellew
Copy link
Collaborator

@vito-c as i mentioned, the tool doesn't add braces. the rule is called redundant braces. maxLines simply specifies that larger code blocks should not be touched. in both cases, you are looking for the opposite of what the tool does.

@kitbellew
Copy link
Collaborator

other examples of why adding braces is going to be hard: #1463 #1222

@vito-c
Copy link
Author

vito-c commented Jun 15, 2020

@kitbellew thanks! I will check this out when I have time :)

@vito-c
Copy link
Author

vito-c commented Jul 18, 2020

I have something working on my local that adds braces when the body of a def function is over a certain maxlines.

It would result in changing some of the output of the tests for example:
rewrite.redundantBraces.maxLines=1
scalafmt-tests/src/test/resources/rewrite/RedundantBraces.stat

<<< #1633 1.3: func with placeholder and infix
object a {
  def findAccountByAPIKey(apiKey: APIKey) = byKeyCache.get(apiKey) match {
    case None =>
      delegate.findAccountByAPIKey(apiKey) map {
         _ map(_) tap (add(apiKey, _)) unsafePerformIO(foo)
      }
  }
}

no longer produces this:

>>>
object a {
  def findAccountByAPIKey(apiKey: APIKey) =
    byKeyCache.get(apiKey) match {
      case None =>
        delegate.findAccountByAPIKey(apiKey) map {
          _ map (_) tap (add(apiKey, _)) unsafePerformIO (foo)
        }
    }
}

it now produces:

>>>
object a {
  def findAccountByAPIKey(apiKey: APIKey) = {
    byKeyCache.get(apiKey) match {
      case None =>
        delegate.findAccountByAPIKey(apiKey) map {
          _ map _ tap (add(apiKey, _)) unsafePerformIO (foo)
        }
    }
  }
}

@kitbellew
Copy link
Collaborator

that's good to hear. one caveat: it must also work for

object a { def findAccountByAPIKey(apiKey: APIKey) = byKeyCache.get(apiKey) match { case None => delegate.findAccountByAPIKey(apiKey) map { _ map(_) tap (add(apiKey, _)) unsafePerformIO(foo) } } }

for that, the change must be in FormatWriter, and it shouldn't look at the number of input lines but only the proposed output.

@vito-c
Copy link
Author

vito-c commented Jul 18, 2020

Is this the test case you are looking for?

<<< add braces def single line
object a { def findAccountByAPIKey(apiKey: APIKey) = byKeyCache.get(apiKey) match { case None => delegate.findAccountByAPIKey(apiKey) map { _ map(_) tap (add(apiKey, _)) unsafePerformIO(foo) } } }
>>>
object a {
  def findAccountByAPIKey(apiKey: APIKey) = {
    byKeyCache.get(apiKey) match {
      case None =>
        delegate.findAccountByAPIKey(apiKey) map {
          _ map (_) tap (add(apiKey, _)) unsafePerformIO (foo)
        }
    }
  }
}

@vito-c
Copy link
Author

vito-c commented Jul 19, 2020

and it shouldn't look at the number of input lines but only the proposed output.

Any tips here?

@kitbellew
Copy link
Collaborator

and it shouldn't look at the number of input lines but only the proposed output.

Any tips here?

if you do it in FormatWriter, you know where the formatter is putting the newlines.

@kitbellew
Copy link
Collaborator

Is this the test case you are looking for?

<<< add braces def single line
object a { def findAccountByAPIKey(apiKey: APIKey) = byKeyCache.get(apiKey) match { case None => delegate.findAccountByAPIKey(apiKey) map { _ map(_) tap (add(apiKey, _)) unsafePerformIO(foo) } } }
>>>
object a {
  def findAccountByAPIKey(apiKey: APIKey) = {
    byKeyCache.get(apiKey) match {
      case None =>
        delegate.findAccountByAPIKey(apiKey) map {
          _ map (_) tap (add(apiKey, _)) unsafePerformIO (foo)
        }
    }
  }
}

Yes, something like that.

@vito-c
Copy link
Author

vito-c commented Aug 8, 2020

I've made some good progress on this but my lack of understanding on the FormatLocation is holding me back. I guess I'm not clear on how the State object and splits work.

For example:

                  // remove space after "{"
                  val split = state.split.withMod(NoSplit)
                  loc.copy(
                    replace = "(",
                    shift = loc.shift - 1,
                    state = state.copy(prev = prevBegState, split = split)
                  )

Is how you would remove a after the { so what does the prevBegState do? Is there any documentation on how to manipulate an Array[FormatLocation] Specifically I'm trying to add a { but I think I've only been able to replace so far.

I've gotten to the point where I can generate the string I want at this line.

and then I was hoping the Formatter would continue on it's way and finish formatting. But I think the way I used the State and split is causing it now to not format correctly.

@vito-c
Copy link
Author

vito-c commented Aug 14, 2020

Ok I am going to attempt to clean up what I have and create a pull request. Do you have anymore test cases I should add? @kitbellew

@vito-c
Copy link
Author

vito-c commented Aug 17, 2020

Couple questions I have to be able to complete this:

  1. How can I change the settings in the middle of a test? For example in .stat file I would like to set the redundant braces maxLines to value for specific tests.
  2. How can I count the number of lines that an Array[FormatLocation] is going to produce? I tried counting isNL on the FormatLocation is that correct?
  3. How can I access the settings inside the FormatWritter it seems like it requires context to get the settings.

@poslegm
Copy link
Collaborator

poslegm commented Aug 17, 2020

<<< my test
maxLines = 80
===
object a {}
>>>
object a {}

more info here

  1. not sure

  2. Yes, context is recommended because settings can be customized for code block

@kitbellew
Copy link
Collaborator

  1. How can I count the number of lines that an Array[FormatLocation] is going to produce? I tried counting isNL on the FormatLocation is that correct?

FormatLocation already has lineId which is monotonic and matches if and only if tokens are on the same line.

@kitbellew
Copy link
Collaborator

@vito-c how's it going? i am planning to release 2.7.0 soon, please share your progress if you'd like it to be included.

@vito-c
Copy link
Author

vito-c commented Aug 25, 2020

@kitbellew what's your eta? I am not quite there yet... The other thing I was thinking about is that people who didn't have braces before will get them added back in when they upgrade which might be problematic.

@poslegm

Yes, context is recommended because settings can be customized

So how do I access the rewrite.redundantBraces.maxLines in the FormatWriter? It looks like it's passed in implicitly in the RedundantBraces... I just hard coded it for now but I need to figure out how to get the context/settings in that class

@kitbellew
Copy link
Collaborator

@vito-c all configuration settings are available via ScalafmtConfig, with local overrides, found inside FormatOps, usually called style. Each FormatLocation should likely have one.

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

Successfully merging a pull request may close this issue.

3 participants