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

Respect the intent of rules authors when returning atomic patches #1261

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

mlachkar
Copy link
Collaborator

In scalafixFileEvaluation, there is a possibility to apply only some patches.
The current issue, is that we were returning the low level patches, by recursively decomposing even Atomic patches.

If the author rule has created an atomic patch that contains two Add patches, we should not decompose it,
and therefore previewPatches() can only be called with "atomic" patches

In scalafixFileEvaluation, there is a possibility to apply only some patches.
The current issue, is that we were returning the low level patches, by recursively decomposing even Atomic patches.

If the author rule has created an atomic patch that contains two Add patches, we should not decompose it,
and therefore `previewPatches()` can only be called with "atomic" patches
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with ?w=1 the diff is really hard to follow, and I feel like many changes are unnecessary or could be extracted to no-op refactoring commits. Is there any chance you could amend it to try to produce a minimal diff? Thanks!

}
}

class CommentFileRule2 extends v1.SyntacticRule("CommentFileRule2") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class CommentFileRule2 extends v1.SyntacticRule("CommentFileRule2") {
class CommentFileAtomic extends v1.SyntacticRule("CommentFileAtomic") {

val fileEvaluation1 = run1.evaluate().getFileEvaluations.head
val patches1 = fileEvaluation1.getPatches
assert(patches1.length == 2)
val obtained1 = fileEvaluation1.previewPatches().get
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no assertion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didi an assertion 

assertNoDiff(obtained1, obtained2)

@@ -214,6 +213,74 @@ class ScalafixArgumentsSuite extends AnyFunSuite with DiffAssertions {
assert(contentAfterRun == fileEvaluation.previewPatches().get)
}

test("ScalafixArguments.evaluate retrieve only atomic patches") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think tests would be more readable (and avoid 1/2 suffixes in variables) if you had more tests with less assertions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

scalafix-core/src/main/scala/scalafix/v0/Rule.scala Outdated Show resolved Hide resolved
@@ -191,6 +200,20 @@ object PatchInternals {
loop(patch)
}

// don't decompose Atomic Patch
def foreachPatchUnit(patch: Patch)(f: Patch => Unit): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very generic signature, yet it's used only for accumulation, maybe the 2 new methods could be merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it twice

Copy link
Collaborator Author

@mlachkar mlachkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I will amend to reduce the diff.

@@ -374,7 +374,7 @@ object MainOps {
ctx: RuleCtx,
index: Option[v0.SemanticdbIndex]
): Option[String] =
Try(tokenPatchApply(ctx, index, patches)).toOption
Try(patchApply(ctx, index, patches)).toOption
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is in fact a rename.

@mlachkar
Copy link
Collaborator Author

I removed all my small refactoring (renames, and removing return keyword).
The only change is about not using underlying that loop inside an Atomic Patch.

@mlachkar mlachkar requested a review from bjaglin October 26, 2020 21:23
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for taking the time to amend, much clearer 👍

@mlachkar mlachkar merged commit 8e213be into scalacenter:master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants