Add methods to scalafix api to be able to retrieve patches

[mlachkar]

I added a ScalafixResult that contains:

• Patches
• Diagnostics
• Error

Applying patches still consist on writing the file in the desired Path if there is a diff.
You can also apply selective patches if needed. The idea is to enable another project that needs this feature.

[adpi2]

This looks very good to me!

[adpi2]

What does this mean?

[mlachkar]

there are 3 modes. this one only checks and doesn't write to file.

[bjaglin]

I finally found some time to look at this promising PR! I only focused on the API declaration & tests without looking at the implementation to keep a naive consumer standpoint as much as possible.

[bjaglin]

run() does not necessarily have that side effect, it depends on the ScalafixMainMode.

I guess only a few modes make sense with this new method, I think it should be documented.

[bjaglin]

do we need to expose a patch as single unit? it's coupled to a file and usually use together with others, so instead I would take a kind or an id as parameter on the apply overloads (since I guess the only purpose of getting them is to filter before applying them?)

[bjaglin]

discussed offline: this PR is driven by a need to try to apply some patches until files reach a certain state (via previewSelectivePatches), but we don't know beforehand which patch will be tried, so we need to be able to address them individually as we try and iterate.

[bjaglin]

I was not expecting to see side-effect methods on a Result object, which seems like the end of the process. I can't really find the right term for it, but I was thinking about something along

Suggested change

	public interface ScalafixResult {
	public interface ScalafixEvaluation {

[bjaglin]

can we fail or feed both rather than ignoring?

[mlachkar]

So if the name is evaluate, it make sense that the Mode or callback are not called ?

[bjaglin]

I think we should fail if evaluate is called on a ScalafixArguments that was explicitly set a mode or a callback, since they have no effect.

[mlachkar]

Evaluate should not fail from my point of view. It's a new way to interact with scalafix, that requires fewer arguments.

[mlachkar]

The interfaces look like that

interface ScalafixArguments {
  ScalafixEvaluation evaluate();
}

interface ScalafixEvaluation {
   ScalafixOutput[] getScalafixOutputs(); // for each file
   ScalafixError[] writeResult();
}
interface ScalafixOutput {
  Path path();
  Rules[] rules;
  ScalafixPatch[] patches();
  ScalafixDiagnostic[] getDiagnostics();
  Optional<String> getUnifiedDiff();

// write to file the result of patches
 void applyAllPatches();
 void applySelectivePatches(patch[] patches);

// and equivalent function that returns the result of applying patches with writing the diff.
 Optional<String> evaluateAllPatches();
 Optional<String> evaluateSelectivePatches(patch[] patches);
 }

[bjaglin]

it looks like an enum

[mlachkar]

you are right, but it's not corresponding to all possible cases in Patch sealed trait.
Creating an Enum may add too much constraint for this field, especially that the only use case that I see right now is applying only patches relate to Import

My opinion is that String is enough for now, but maybe later we can add an enum.

[bjaglin]

the only use case that I see right now is applying only patches relate to Import

Can you elaborate the use-case? maybe we don't need to expose the path class hierarchy as-is, but just what we need - I am concerned we are leaking internals here.

[mlachkar]

If we apply a rule that looks like explicitResultType: When a type is added, sometimes an import is also added.
We would like to be able to apply all import patches first.
I understand your concern. We can maybe expose 3 kinds: ReplaceSymbol, AddImport, RemoveImport?
I can also remove kind until it's clearer what we would need, and add this in a second PR later.

[bjaglin]

We would like to be able to apply all import patches first.

I am not very familiar with the patch internals, but doesn't https://scalacenter.github.io/scalafix/docs/developers/patch.html#atomic allow to group patches that should be applied "together" - maybe we need to expose patches as group of atomic patches?

[bjaglin]

I can also remove kind until it's clearer what we would need, and add this in a second PR later.

That would probably be better as there is already a lot of surface in the new APIs. ScalafixPatch would then become an empty trait for now, highlighting the fact that it's used for its identify (to iterate and converge on a proper state), rather than for its attributes.

[bjaglin]

output was not really clear to me since it's the name of the class itself, while I think here it means the content of the path after the fix is applied. Also, the fix does not really show that this is a dry-run version of applyPatches. What about

interface ScalafixTargetEvaluation {
  Path getTarget(); 
  Optional<String> previewPatches();

(we should stick to one wording for file/target/path to designate what scalafix runs on by the way - not sure what's the best or what's already used)

[bjaglin]

how does this relate to the getErrors ?

[mlachkar]

It's a global message error that can be empty. Errors are a concatenation of code errors that result from a merge function.

[bjaglin]

what do you mean by "if not"?

[bjaglin]

Just a few last comments on the API, but overall LGTM. I haven't looked at the implementation, but this can/will evolve as you use it I guess, so please don't wait for me.

I am still a bit concerned about the opaque ScalafixPatch object that is exposed to be able to apply/preview a random subset of patches, but I don't have a better idea to tackle this use-case.

[bjaglin]

can this be tested? I don't see a check in the implementation

[mlachkar]

Yes, I will add test for this.

[mlachkar]

Thank you for all reviews. I will merge it now.