Move patch operations into RewriteCtx, fixes #193. #235
Conversation
This avoid the need for extension methods. The implementations of the patch operations are now abstract to make it easier to read.
|
Quite nice to see no changes were necessary on the rewrites. I was tempted to mix the operations into tree.replaceWith(newTree)The RewriteCtx would need to be implicit in that case. However, I personally prefer the |
The distinction is no longer buying us anything now that semantic patch ops take an implicit mirror as arguments.
|
Nice! I'll review it soon, in the meantime the CI is failing because there's a faulty reference in the readme. |
|
I'm about to finish #192 soon, which follows up on this PR. |
| case class RewriteCtx(tree: Tree, config: ScalafixConfig) { | ||
| def syntax = | ||
| case class RewriteCtx(tree: Tree, config: ScalafixConfig) extends PatchOps { | ||
| ctx => |
There was a problem hiding this comment.
this is just an alias for this right?
There was a problem hiding this comment.
Yes, but I like the convention of calling it ctx. The ctx is not much used to implement the patches at the moment but I suspect that later we may need it to support configuration etc.
| def addGlobalImport(importer: Importer): Patch = AddGlobalImport(importer) | ||
| // Semantic patch ops. | ||
| def removeGlobalImport(symbol: Symbol)(implicit mirror: Mirror): Patch | ||
| def addGlobalImport(importer: Importer)(implicit mirror: Mirror): Patch |
There was a problem hiding this comment.
I think this makes a lot of sense 👍
|
Thank you for the review! |
This avoid the need for extension methods. The implementations of the
patch operations are now abstract to make it easier to read.