Fix #373: addLeft's implementation doesn't match its specification #378
Conversation
|
The binary compatibility test ("mima"?) fails (because I renamed the test suite). Is that a blocker? |
There was a problem hiding this comment.
Thank you for this contribution @abeln ! We recently enabled MiMa in an attempt to stabilize the public API.
A few minor comments, otherwise looks great. Nice job refactoring the test suite to make it easier to validate this fix.
| @@ -8,16 +8,20 @@ import scalafix.syntax._ | |||
|
|
|||
| import org.scalatest.FunSuiteLike | |||
|
|
|||
| class SyntacticRuleSuite(rule: Rule) extends FunSuiteLike with DiffAssertions { | |||
There was a problem hiding this comment.
The migration manager (mima) is complaining because of this change, scalafix-testkit is a published artifact to help scalafix users develop custom rules in repositories outside of scalacenter/scalafix.
I think we can make this change without breaking compatibility by changing this parameter to defaultRule: Rule = Rule.empty.
| @@ -8,16 +8,20 @@ import scalafix.syntax._ | |||
|
|
|||
| import org.scalatest.FunSuiteLike | |||
|
|
|||
| class SyntacticRuleSuite(rule: Rule) extends FunSuiteLike with DiffAssertions { | |||
| def check(name: String, original: String, expected: String): Unit = { | |||
There was a problem hiding this comment.
Forward this to new check overload with defaultRule
| test(name) { | ||
| import scala.meta._ | ||
| val obtained = rule.apply(Input.String(original)) | ||
| assertNoDiff(obtained, expected) | ||
| } | ||
| } | ||
|
|
||
| def checkDiff(original: Input, expected: String): Unit = { |
There was a problem hiding this comment.
Forward this to new checkDiff overload with defaultRule
| test("on parse error") { | ||
| intercept[ParseException] { | ||
| ProcedureSyntax.apply(Input.String("object A {")) | ||
| } | ||
| } | ||
| } | ||
| class PatchSuite | ||
| extends SyntacticRuleSuite(Rule.syntactic("PatchSuite")(ctx => |
There was a problem hiding this comment.
This is a nice cleanup, removing the rule definition in the constructor 👍
| ctx.addRight(ctx.tree.tokens.find(_.is[Ident]).get, "bba") | ||
| } | ||
|
|
||
| val addLeftRule: Rule = Rule.syntactic("addLeft") { (ctx) => |
There was a problem hiding this comment.
Can we move this definition right next to the next where it's used?
Return `headOption` instead of `lastOption` in addLeft's implementation. Additionally, add a unit test, and refactor the syntax tests. TESTED=unit tests pass
|
Thanks for the review. PTAL. |
|
The binary compatibility test now passes. |
There was a problem hiding this comment.
LGTM 👍 Thank you @abeln ! I will immediately be using the new PatchSuite to test a fix for #372
FYI, I will rename defaultRule to rule in a followup PR, I just realized although this change is binary compatible it does break clients that use a named argument. I am a total beginner at keeping compatibility between releases, but I'm trying to learn 😅
Return
headOptioninstead oflastOptionin addLeft's implementation.Additionally, add a unit test, and refactor the syntax tests.
TESTED=unit tests pass