#690: Support check for "redundant enclosing brace" in string interpo... #703

ysusuk · 2017-01-29T14:28:03Z

…lation.

olafurpg · 2017-01-29T14:33:17Z

Thank you for this contribution! Great work.

I think there's a missing test case s"${foo}bar". Removing the curly brace will result in the ast to change.

ysusuk · 2017-01-29T14:37:01Z

Thank you @olafurpg!

ysusuk · 2017-01-29T17:06:39Z

can you suggest me a way to find out what is going after closing brace in s"${foo}bar". i tried

case t @ Term.Name(name) =>
  val closeBrace = nextToken(t.tokens.head)
  val afterCloseBrace = nextToken(closeBrace)

but it doesn't seems to work.

olafurpg · 2017-01-29T17:21:32Z

Maybe something like this?

@ val t = q""" q"$${foo}bar$$banana ok" """; t.parts.tail.zip(t.args)
t: Term.Interpolate = q"$foobar$banana ok"
res9_1: collection.immutable.Seq[(Lit, Term)] = List(("bar", foo), (" ok", banana))

olafurpg · 2017-01-29T17:23:29Z

Note that there are Interpolation.Splice{End,Start} tokens that you may bump into

@ q""" q"$${foo}bar$$banana ok" """.tokens.map(x => x.getClass -> x.structure)
res12: collection.immutable.IndexedSeq[(Class[?0], String) forSome { type ?0 <: Token }] = Vector(
  (class scala.meta.tokens.Token$BOF,BOF [0..0)),
  (class scala.meta.tokens.Token$Interpolation$Id,q [0..1)),
  (class scala.meta.tokens.Token$Interpolation$Start," [1..2)),
  (class scala.meta.tokens.Token$Interpolation$Part, [2..2)),
  (class scala.meta.tokens.Token$Interpolation$SpliceStart,$ [2..3)),
  (class scala.meta.tokens.Token$Ident,foobar [3..9)),
  (class scala.meta.tokens.Token$Interpolation$SpliceEnd, [9..9)),
  (class scala.meta.tokens.Token$Interpolation$Part, [9..9)),
  (class scala.meta.tokens.Token$Interpolation$SpliceStart,$ [9..10)),
  (class scala.meta.tokens.Token$Ident,banana [10..16)),
  (class scala.meta.tokens.Token$Interpolation$SpliceEnd, [16..16)),
  (class scala.meta.tokens.Token$Interpolation$Part, ok [16..19)),
  (class scala.meta.tokens.Token$Interpolation$End," [19..20)),
  (class scala.meta.tokens.Token$EOF,EOF [20..20))
)

olafurpg · 2017-01-30T12:09:15Z

core/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala

+            val openBrace = prevToken(arg.tokens.head)
+            val closeBrace = nextToken(arg.tokens.head)
+            (openBrace, closeBrace, value) match {
+              case (LeftBrace(), RightBrace(), "") =>


Why ""? Have you tested cases like?

q"" q"$a" q"$a$b" q" $a " q"$a-" q"${a}1" q"${a}_2"

Hi @olafurpg. I added all the test cases you mentioned. I'm just not sure that it does make big sense to test all the cases without braces! Since there is a check that args are inside braces! maybe one test here is enough and then anybody that will make a review will not get lost between them.

Whoops, testing without braces is not so interesting I agree. What if those examples are expected outputs and the inputs have variables wrapped in braces? I still don't understand why it should only rewrite when value is ""

I suspect the current implementation misses some opportunities to rewrite

olafurpg · 2017-01-30T12:11:04Z

core/src/test/resources/rewrite/RedundantBraces.stat

+}
+<<< enclosing braces in str interpolation
+object a {
+  def b(d: Int) = s"Hello ${d.toString}"


I don't think the def b(d: Int) = part is necessary. Maybe a block is sufficient

{ s"$a" }

Note that the test doesn't have to type check, only parse.

it doesn't work without object. max what i achieved is

<<< redundant enclosing braces in str interpolation object a { s"Hello ${value}" } >>> object a { s"Hello $value" }

olafurpg · 2017-01-30T15:50:54Z

core/src/test/resources/rewrite/RedundantBraces.stat

 }
 >>>
 object c {
-  s" $a "
+  s" ${a} "


Is this expected output?

it's a bug)

olafurpg · 2017-01-30T15:51:23Z

core/src/test/resources/rewrite/RedundantBraces.stat

+}
+>>>
+object c {
+  s"${a}-"


Same here, a can be unwrapped.

and this one fails s"${a} a${b}b"

i was generally thinking about checks like value.isEmpty or starts with \n, \r or anything like this! what else should it be? minus/plus... is there kind a list of them? or do you think there is a better approach?

You can look at the spec https://www.scala-lang.org/files/archive/spec/2.11/01-lexical-syntax.html#identifiers

Seems like this rewrite should not take place when the right hand part starts with underscore or an alphanumeric character. In other cases, you can safely remove the curly brace.

ysusuk · 2017-01-30T17:09:27Z

Thanx a lot for support @olafurpg! i corrected logic and cleaned up tests!

codecov-io · 2017-01-30T17:35:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@abf8ba2).

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abf8ba2...0c8173f. Read the comment docs.

olafurpg

Almost ready! :)

olafurpg · 2017-01-30T18:35:12Z

core/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala

@@ -49,10 +46,27 @@ object RedundantBraces extends Rewrite {
    bodyIsNotTooBig
  }

+  val ALPHA_NUMERIC_AND_UNDERSCORE = ((('a' to 'z') ++ ('A' to 'Z') ++ ('0' to '9')) :+ '_').toSet


What about variables like æ1?

res0: Boolean = true @ Character.isLetterOrDigit('-') res1: Boolean = false @ Character.isLetterOrDigit('æ') res3: Boolean = true

olafurpg · 2017-01-30T18:36:49Z

core/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala

    val builder = Seq.newBuilder[Patch]
    code.collect {
+      case t: Term.Interpolate if settings.stringInterpolation =>
+        t.parts.tail.zip(t.args).collect {
+          case (Lit(value: String), arg @ Term.Name(name)) =>


def isIdentifierStart(value: String) = value.isEmpty || Character.isLetterOrDigit(value.head) || value.head == '_' ... case xxx if !isIdentifierStart(value) =>

is more like this

def isIdentifierStart(value: String) = value.nonEmpty && (Character.isLetterOrDigit(value.head) || value.head == '_')

when value is empty we don't want to skip patch

olafurpg · 2017-01-30T18:37:26Z

core/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala

+          case (Lit(value: String), arg @ Term.Name(name)) =>
+            val openBrace = prevToken(arg.tokens.head)
+            val closeBrace = nextToken(arg.tokens.head)
+            (openBrace, closeBrace, value.headOption) match {


remove , value.headOption

olafurpg · 2017-01-30T18:40:44Z

core/src/test/resources/rewrite/RedundantBraces.stat

+}
+<<< not redundant enclosing braces in str interpolation (right part starts with underscore)
+object a {
+  s"Hello ${foo}_bar"


Maybe also test s"${foo}æ"

added, thank you!

olafurpg

LGTM 👍 just minor nitpick remaining (I hope you don't mind my endless comments!)

olafurpg · 2017-01-31T18:25:44Z

core/src/main/scala/org/scalafmt/rewrite/RedundantBraces.scala

-                builder += Patch(openBrace, closeBrace, name)
-              case (LeftBrace(), RightBrace(), None) =>
+            (openBrace, closeBrace) match {
+              case (LeftBrace(), RightBrace()) if !isIdentifierAtStart(value) =>


can we move the !isIdentifierAtStart(value) guard to the case Lit(value... line?

olafurpg · 2017-01-31T18:27:28Z

core/src/test/resources/rewrite/RedundantBraces.stat

@@ -192,6 +192,14 @@ object a {
 object a {
  s"Hello ${foo}_bar"
 }
+<<< not redundant enclosing braces in str interpolation (right part starts with special symbol)


Can we move all of the "in str interpolation" tests into a separate file named RedundantBracesStrInterpolation.stat? Also maybe remove the "redundant braces" part since it's implicit for that test suite, I prefer to keep test names as short as possible.

…t file

olafurpg · 2017-01-31T21:57:29Z

Yay \o/ thanks for going through all the comments.

I will try to get a release out tomorrow

#690: Support check for "redundant enclosing brace" in string interpo…

02a2e8f

…lation.

bar text case added

6dbb87a

olafurpg reviewed Jan 30, 2017

View reviewed changes

Yuriy Susuk added 3 commits January 30, 2017 17:02

additional test casses

563c76d

all expressions are wrapped into object

0575418

most argss in input are wrapped in braces

8d16d4a

olafurpg reviewed Jan 30, 2017

View reviewed changes

Yuriy Susuk added 2 commits January 30, 2017 19:04

defined conditions when remove the braces

cc87132

test name corrected

a75c69c

olafurpg reviewed Jan 30, 2017

View reviewed changes

Character.isLetterOrDigit is used instead of self made func

723f4a3

olafurpg approved these changes Jan 31, 2017

View reviewed changes

tests for redundant braces in str interpolation moved to separate tes…

0c8173f

…t file

olafurpg merged commit 932b45f into scalameta:master Jan 31, 2017

olafurpg mentioned this pull request Mar 5, 2017

Support check for "redundant enclosing brace" in string interpolation. #690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#690: Support check for "redundant enclosing brace" in string interpo... #703

#690: Support check for "redundant enclosing brace" in string interpo... #703

ysusuk commented Jan 29, 2017

olafurpg commented Jan 29, 2017

ysusuk commented Jan 29, 2017

ysusuk commented Jan 29, 2017 •

edited

Loading

olafurpg commented Jan 29, 2017

olafurpg commented Jan 29, 2017

olafurpg Jan 30, 2017 •

edited

Loading

ysusuk Jan 30, 2017 •

edited

Loading

olafurpg Jan 30, 2017

olafurpg Jan 30, 2017

olafurpg Jan 30, 2017

olafurpg Jan 30, 2017

ysusuk Jan 30, 2017 •

edited

Loading

olafurpg Jan 30, 2017

ysusuk Jan 30, 2017

olafurpg Jan 30, 2017

ysusuk Jan 30, 2017

ysusuk Jan 30, 2017 •

edited

Loading

olafurpg Jan 30, 2017

ysusuk commented Jan 30, 2017

codecov-io commented Jan 30, 2017 •

edited

Loading

olafurpg left a comment

olafurpg Jan 30, 2017

olafurpg Jan 30, 2017

ysusuk Jan 30, 2017

olafurpg Jan 30, 2017

olafurpg Jan 30, 2017

ysusuk Jan 30, 2017

olafurpg left a comment

olafurpg Jan 31, 2017

olafurpg Jan 31, 2017

olafurpg commented Jan 31, 2017

#690: Support check for "redundant enclosing brace" in string interpo... #703

#690: Support check for "redundant enclosing brace" in string interpo... #703

Conversation

ysusuk commented Jan 29, 2017

olafurpg commented Jan 29, 2017

ysusuk commented Jan 29, 2017

ysusuk commented Jan 29, 2017 • edited Loading

olafurpg commented Jan 29, 2017

olafurpg commented Jan 29, 2017

olafurpg Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

ysusuk Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysusuk Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysusuk Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysusuk commented Jan 30, 2017

codecov-io commented Jan 30, 2017 • edited Loading

Codecov Report

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Jan 31, 2017

ysusuk commented Jan 29, 2017 •

edited

Loading

olafurpg Jan 30, 2017 •

edited

Loading

ysusuk Jan 30, 2017 •

edited

Loading

ysusuk Jan 30, 2017 •

edited

Loading

ysusuk Jan 30, 2017 •

edited

Loading

codecov-io commented Jan 30, 2017 •

edited

Loading