Remove reflection from KtLintStep test.

[runningcode]

Reflection makes it really hard to maintain and add new features.
This makes the code much easier to read and maintain.

Let me know if this is okay or not. I wasn't too clear on the reasoning on why it was this way before.

[nedtwigg]

Spotless works with a ton of different formatting libraries (off the top of my head: eclipse, google-java-format, ktlint, scalafmt, greclipse, freshmark). For each formatting library, we're also compatible with many versions of it, so that users can upgrade or downgrade versions without relying on Spotless cutting a new release. For each formatter, we resolve a special classpath for just that lone formatter and its deps.

The approach used in this PR would break quickly if you made the same changes for the other formatters - the transitive deps would clash, not to mention trying to add support for multiple versions. I agree the reflection is ugly and hard to maintain, but it's an overall win because it greatly reduces the amount of maintaining that needs to be done.

[runningcode]

Thanks for the explanation. What do you think of using a compileOnly dependency? That way the logic can be built without reflection and without packaging the dependency in the release jar?

[jbduncan]

What do you think of using a compileOnly dependency?

I'm kind of curious about this too.

I wonder if the reason this approach hasn't been tried yet is because if two formatter libraries A and B depend on a library C, but the versions of C they import are incompatible with one another, then Gradle will automatically choose one version of C for both A and B to use, but then either A and B will fail or do their jobs incorrectly.

Is my understanding correct @nedtwigg? Or is there something else or more to it than that?

[nedtwigg]

That's one of the issues. The compileOnly trick is a handy one for cases like this. Goomph, the plugin that makes gradlew ide work, uses it to run code that runs inside the Eclipse IDE, even though those jars are not deps of the Goomph plugin itself:

• build.gradle
• ProjectImporterInternal (written using compileOnly jars)
• ProjectImporter (calls ProjectImporterInternal).

There are a quite a few gotchas though:

1. The code that calls the compileOnly-written code can't import it. If it imports it, that will trigger the compileOnly-written code to import, and you'll get NoClassDef errors. That's why ProjectImporter uses super("com.diffplug.gradle.oomph.ProjectImporterInternal") instead of super(ProjectImporterInternal.class.getName())
2. Every single compileOnly-written formatter will need its own project, because their transitive dependencies conflict. Managing 10 different maven artifacts would be a pain, so we'd need to setup shadow to mash them all back into one. The combination of 1 and 2 mean that there would lots of cases where something works in the IDE, but breaks in the compiled artifact. We could setup tests to catch it before we ship, but they'll be a bit confusing to debug.
3. Each incompatible version bump of each compileOnly-written formatter will need its own project. Right now the same code can support multiple incompatible versions of a formatter lib, which would require separate projects in the compileOnly world. Granted, I think we only use this capability for the ScalaFmtStep at present.

For any single FormatterStep, the pros of compileOnly outweigh the cons. For the project as a whole, I think the cons outweigh the pros.

My recommendation is:

• add the library you're working with as a compile dependency
• write the FormatterStep and tests, make sure it does what you want
• turn the function calls into reflection
• remove the compile depency

I'm open to exploring the compileOnly option, but I'll only have time to point out the problems with it, I don't have time to help fix them. Here are the requirements for switching to compileOnly:

• Setup shadow to mash the jars back into one
• Make sure that none of the compileOnly jars are leaking into the POM
• Figure out how to run the integration tests on the shadow result
• Make it easy to work with in eclipse
• Migrate all reflection-based-formatters to compileOnly. It would be confusing for new contributors to have half one way and half another.

If all of this was complete, but the final result was harder to maintain than the previous result, I would be hesitant to merge it. It seems likely that the extra magic and complexity in the build (which is already pretty complex) would outweigh the simplicity in the FormatterStep (which are already pretty simple). I'll try to keep an open mind, but I'm nervous because it would be so much work for whoever contributes it, but even so I'll be stuck keeping the build running and releasing for the next N years, so I'm pretty conservative about changes that will make that more tedious.

[jbduncan]

Oh wow, I hadn't realised there was so much that could go wrong with a compileOnly solution. Thank you very much for the extensive explanation!