Support custom rulesets and reporters on Java 9+

[jacquerie]

This PR is pretty much a rebase on current master of the first commit in #351. The idea is to reduce the scope so that we can merge and get a release as fast as possible, as I need ktlint to support Java 9+.

[jacquerie]

Hey maintainers! Thanks for the effort you've invested so far in building and taking care of this library. May I ask for feedback on this PR, please? Many apologies for putting pressure on you.

[JLLeitschuh]

Are there tests that can verify this change?

[jacquerie]

Are there tests that can verify this change?

The original PR (#351) included some tests, but in order to achieve that it refactored significant portions of the code to improve testability, something which I gather the maintainers didn't like. Please let me know if you think it's possible to add tests while avoiding the refactoring!

[shashachu]

#351

I think the only question about your previous PR was that it included some gradle upgrades, which seemed outside the scope of the classloader change.

[jacquerie]

I think the only question about your previous PR was that it included some gradle upgrades, which seemed outside the scope of the classloader change.

Oh, if that's the case, then I will rebase some more commits by @marschwar on top of current master, but I will exclude the ones that update gradle or modify the build!

[jacquerie]

Oh, if that's the case, then I will rebase some more commits by @marschwar on top of current master, but I will exclude the ones that update gradle or modify the build!

This happened! Some updates were necessary, but most of @marschwar's code is there.

For example, I couldn't find a way to preserve the following test (https://github.com/marschwar/ktlint/blob/78b63624c1d6f0767d321727452c0207e5d8f09a/ktlint/src/test/kotlin/com/github/shyiko/ktlint/internal/RuleSetLoaderKtTest.kt#L26-L38) because I couldn't find a ruleset published on Maven compatible with 0.34.0. On the other hand, I decided it was not very important, because that feature (remote resolving of dependencies) will be gone once #451 is fixed.

[jacquerie]

Hi @shashachu ! Sorry to bother you again, but we'd really love to use the custom ruleset feature on Java 9+. The fact that this PR adds code to the deprecated MavenDependencyResolver isn't ideal, but I see a path to fix #451 once this PR is merged: we could still benefit from a DependencyResolver class that contains the common resolution logic shared by the loading of rulesets and reporters.

[shashachu]

Hi @shashachu ! Sorry to bother you again, but we'd really love to use the custom ruleset feature on Java 9+. The fact that this PR adds code to the deprecated MavenDependencyResolver isn't ideal, but I see a path to fix #451 once this PR is merged: we could still benefit from a DependencyResolver class that contains the common resolution logic shared by the loading of rulesets and reporters.

Hi! I am actually planning on removing the MavenDependencyResolver for 0.35.0. Once that PR is up, we can revisit this. Sorry for the thrash.

[jacquerie]

As expected, merging #566 caused conflicts for this PR. Now, do you like the solution that I proposed in #495 (comment) (adding a DependencyResolver class containing the common resolution logic), or would you prefer to see something different?

[shashachu]

As expected, merging #566 caused conflicts for this PR. Now, do you like the solution that I proposed in #495 (comment) (adding a DependencyResolver class containing the common resolution logic), or would you prefer to see something different?

I don't have a problem with it assuming there's still enough code left to warrant a separate class. Making the code cleaner never seems like a bad idea.

[Tapchicoma]

I may be wrong, but only problem to support Java 9+ seems to be this hack to load 3rd party jars:

ktlint/ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt

Line 512 in b63cd7f

val method = URLClassLoader::class.java.getDeclaredMethod("addURL", URL::class.java)

Most probably, using this (picked from your changes, replace RuleSet with Reporter when required):

val rulesetClassLoader = URLClassLoader(urls.toTypedArray(), RuleSet::class.java.classLoader)

would solve the problem.

[jacquerie]

I may be wrong, but only problem to support Java 9+ seems to be this hack to load 3rd party jars

It blows up a little earlier, when trying to cast the system class loader as a URLClassLoader. But I think you put me on a promising track: WDYT of jacquerie@032670c ?

[Tapchicoma]

It blows up a little earlier, when trying to cast the system class loader as a URLClassLoader. But I think you put me on a promising track: WDYT of jacquerie/ktlint@032670c ?

I am not an expert in classloaders, but seems it is the right way to go.