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

Improvement: IntelliJ plugin with configurable implementation #18

Merged
merged 56 commits into from
Oct 15, 2019

Conversation

dansanduleac
Copy link
Contributor

@dansanduleac dansanduleac commented Oct 14, 2019

Before this PR

The idea-plugin just bundles the current version of the formatter.

However, the plugin version is global to an IntelliJ installation, so all projects would be forced to use that version of the formatter. This is bad because IntelliJ formatting could get out of sync with ./gradlew format

After this PR

==COMMIT_MSG==
IntelliJ plugin can be automatically configured with a custom version of the formatter, by applying com.palantir.java-format in gradle and setting the palantirJavaFormat.implementationVersion.
==COMMIT_MSG==

In order to achieve this, we've separated palantir-java-format-api out of the formatter. This is a super thin API layer, and the implementation is found by service loading it out of the implementation jar.

Possible downsides?

@dansanduleac dansanduleac requested a review from ferozco October 14, 2019 15:33
@policy-bot policy-bot bot requested a review from iamdanfox October 14, 2019 15:33
@dansanduleac dansanduleac removed the request for review from iamdanfox October 14, 2019 15:37
Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

small nits then should be good to go! super excited about this :)

version = "191.5849.21"
pluginName = "palantir-java-format"
updateSinceUntilBuild = true
version = "191.5849.21"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to set this to 2019.1 or 2019.2 so we're testing against latest?

version = project.version
sinceBuild = '173'
untilBuild = ''
pluginDescription = "Formats source code using the palantir-java-format tool. This version of " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Description is a little lame now that the version can be configured

}

publishPlugin {
token = project.ext.properties.jetbrainsPluginRepoToken
token = project.ext.properties.jetbrainsPluginRepoToken
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this? do we plan on publishing to intellij?

@@ -102,5 +128,17 @@ public String getEnabled() {
return null;
}
}

@Override
public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should update or remove this

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

small nits then should be good to go! super excited about this :)

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

small nits then should be good to go! super excited about this :)

@iamdanfox
Copy link
Contributor

iamdanfox commented Oct 15, 2019

@dansanduleac and I have been pairing to try and make the :palantir-java-format-api a bit nicer - I think we might want to iterate further, but would prefer to put that in another PR, so approving!

@iamdanfox iamdanfox merged commit 1a648bb into develop Oct 15, 2019
@iamdanfox iamdanfox deleted the fo/ds/gradle-intellij-integration branch November 14, 2019 14:29
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.

3 participants