-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fix the intellij plugin when the bundled formatter is used #619
Conversation
Generate changelog in
|
return listDirAsUrlsUnchecked(pathToUse); | ||
Path implDir = PalantirCodeStyleManager.PLUGIN.getPath().toPath().resolve("impl"); | ||
log.debug("Using palantir-java-format implementation bundled with plugin: {}", implDir); | ||
return listDirAsUrlsUnchecked(implDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverts a change that went in yesterday, which didn't do anything
@@ -73,8 +74,8 @@ tasks.withType(org.jetbrains.intellij.tasks.PrepareSandboxTask) { | |||
dependsOn configurations.runtimeClasspath | |||
|
|||
// Also pack the formatter in its own directory | |||
into("${pluginName}/impl") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable was not resolved, so the current version was no longer included in our plugin!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! Interesting that this worked before!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure when it broke since it only failed when the idea plugin used a newer version than the repo
into("${pluginName}/impl") { | ||
from configurations.formatter | ||
into("${name}/impl") { | ||
from configurations.formatter.filter { !configurations.runtimeClasspath.contains(it) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, I ran into issues where dependencies of the formatter where missing when loading the formatter from the impl directory.
Not sure if we have integration tests for this. Can quickly try if the intellij bundled formatter still works after this. Wouldn't be surprised if we can re-add this. I think me removing this filter was mostly a blind guess in trying to fix those issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've verified that it works locally. I was worried that I was testing the wrong path, so I renamed the impl
directory, restarted my IDE, and it failed to find the formatter impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to revalidate using jdk16+ targets though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg, sorry for the merge then!
Released 2.12.0 |
==COMMIT_MSG==
Fix the intellij plugin when the bundled formatter is used
==COMMIT_MSG==