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

Upgrade to kotlin 1.1.3-2 #64

Merged
merged 2 commits into from
Jul 6, 2017
Merged

Conversation

jeremymailen
Copy link
Contributor

Using an older version of kotlin should not be an issue when using ktlint as a standalone tool. However when using the core engine embedded in a gradle plugin an upgrade is necessary to avoid a breaking change in KotlinCoreEnvironment. Reference:

jeremymailen/kotlinter-gradle#13

Thanks!

@shyiko
Copy link
Collaborator

shyiko commented Jul 6, 2017

Hi Jeremy.

Have you considered loading ktlint in a separate class-loader? (ktlint artifact + all its dependencies (transitive or not) would have to be registered & resolved as part of a separate configuration at runtime) The thing is, I had my share working with Intellij APIs and one thing to be certain of - things break all the time (backward compatibility is not honored at all (technically compiler API isn't public so we can't complain here but it does mean that we should expect even "more-than-usual" events of breakage)). Also, unless compilers are separated there will always be a chance of stuff blowing up for no obvious reason. So while I understand that separation-with-class-loader entails slightly more work - I bet it will pay off in the long run.

Anyway, thank you for the PR. I'm merging it in but I won't be able to publish a new version right away (1.1.X+ compiler requires JDK 8 and I was hoping to combine it with a bunch of new rules (that I'm live-testing right now) before turning it into a new release). Unless, of course, you object :)

@shyiko shyiko merged commit 0b2f004 into pinterest:master Jul 6, 2017
@jeremymailen
Copy link
Contributor Author

Thanks for the good advice. Definitely follow the roadmap you have planned!

I did spend a few hours a while back trying to get the lint and format gradle tasks running in their own classloader. I was trying to implement choosing ktlint version via the kotlinter extension. It turned out to be complicated given the gradle buildscript lifecycle. I might succeed if I tried again :).

The other option would be running it out of process with JavaExec, but I'd lose the ability to do interesting things with the results for each file in gradle-space. Also harder to implement per-file incremental build if I were to add that feature.

I wouldn't want to suggest more work for you, but ktlint could call into the compiler services using an interface and instantiate the correct adapter flavor using reflection similar to spring boot. While it doesn't immediately feel beneficial for the tool, if your library continues to gain popularity like it has people might find this handy for building deeper integrations.

@shyiko
Copy link
Collaborator

shyiko commented Jul 7, 2017

@jeremymailen Thank you 🙇‍♂️. I'll consider it.

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.

2 participants