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

Java 9+ compatibility #351

Closed
wants to merge 10 commits into from
Closed

Java 9+ compatibility #351

wants to merge 10 commits into from

Conversation

marschwar
Copy link

This PR attempts to resolve #259.

It looks like much more than it actually is. Essentially I changed.

(ClassLoader.getSystemClassLoader() as java.net.URLClassLoader)
    .addURLs(artifactUrls)

to

URLClassLoader(artifactUrls.toTypedArray(), parentClassloader)

All the other changes are basically refactoring to be able to write a test for it. I tried to make small commits so there is a chance for someone else to review the changes. Unfortunately I find it really hard to make sure I did not break anything.

One more thing: In order to test that a custom ruleset is downloaded via maven, I added the first one I could find (https://github.com/gabrielittner/ktlint-rules). This obviously is not ideal. I hope someone else has a better idea.

@@ -5,6 +5,7 @@ skip_tags: true
environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is no longer in master. Can you remove this file?

@jaredsburrows
Copy link
Contributor

If you are attempting to solve #259, can you remove changes that are unrelated such as upgrading gradle?

@marschwar
Copy link
Author

Sure. As there was no feedback up until now I did not bother to rebase onto master. If you think it is going in the right direction, I would be happy to do so. If not I would close the PR.

@jaredsburrows
Copy link
Contributor

Usually, larger the PRs or greater the impact may take more time. Keeping the change small and specific will help code reviews go faster :).

@marschwar marschwar closed this Jun 8, 2019
@jacquerie
Copy link

According to https://blog.codefx.org/java/java-9-migration-guide/#Casting-To-URL-Class-Loader, this is not completely straightforward:

If you’ve used the URLClassLoader to dynamically load user provided code (for example as part of a plugin infrastructure) by appending to the class path, then you have to find a new way to do that as it can not be done with Java 9.

So, perhaps, some additional complexity introduced by this PR might be warranted. I can take care of rebasing and removing extraneous stuff such as style changes and version bumps!

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.

--reporter/--ruleset Java 9+ compatibility
3 participants