-
Notifications
You must be signed in to change notification settings - Fork 856
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
Eclipse plugin #118
Eclipse plugin #118
Conversation
Some rough edges remain: - declare maven dependency in plugin module (using manual copy for now) - publish packaged jar (copy google-java-format-eclipse-plugin-0.0.1.jar to your Eclipse dropins folder) - support other (older/newer) Eclipse versions - documentation #52 #52 (comment)
eclipse_plugin/.classpath
Outdated
@@ -4,6 +4,6 @@ | |||
<classpathentry kind="con" path="org.eclipse.pde.core.requiredPlugins"/> | |||
<classpathentry kind="src" path="src"/> | |||
<classpathentry exported="true" kind="lib" path="lib/guava-19.0.jar"/> | |||
<classpathentry exported="true" kind="lib" path="lib/google-java-format-1.1-SNAPSHOT.jar"/> | |||
<classpathentry exported="true" kind="lib" path="lib/google-java-format-1.3-SNAPSHOT.jar"/> | |||
<classpathentry kind="output" path="target/classes"/> |
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 guess we would like to release it based on current release 1.2, right?
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.
1.2 does not contain the just open sourced SnippetFormatter
class: 4ae067a
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 you need an official release, @cushon might create 1.3 "soon", I guess.
eclipse_plugin/pom.xml
Outdated
</description> | ||
|
||
<properties> | ||
<!-- Tycho 0.25 requires Java 8 to run --> |
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.
Hmm, this comment seems to be in conflict with eclipse_plugin/META-INF/MANIFEST.MF
, which AFAICT explicitly states that the Eclipse plugin requires Java 8.
Is the manifest incorrect? If it's right, what's stopping you from using Tycho 0.25+?
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.
Back then, in a galaxy far away when I created the inital eclipse plugin PR, GJF used to depend on Java 7. I didn't check, if an update to Tycho 0.25+ was possible today. Will check later.
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.
Upgraded to 0.26, worked locally. Let's see what Travis and AppVeyor will do with it.
} | ||
return edit; | ||
} | ||
} |
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.
Has this file been formatted with google-java-format itself?
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.
May be. Just c&p it from: https://gist.github.com/cushon/7c17d1212cb708321e7735624aa473bb
Will give it format treatment, later.
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.
Now, this file is formatted with google-java-format. (-:
pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>com.google.errorprone</groupId> | ||
<artifactId>error_prone_annotations</artifactId> | ||
<version>2.0.8</version> | ||
<scope>compile</scope> |
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.
What is this change for?
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.
iirc, it was a try of copying only the needed runtime dependencies. See #52 (comment) ... i'll remove it, if it was/is no longer necessary.
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 think compile is the default, and it means it's on the compilation and runtime classpath. You might be able to do something with optional, but I'd rather leave this as-is unless it's causing problems.
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.
Removing both <scope>compile</scope>
entries.
<id>copy-dependencies</id> | ||
<phase>package</phase> | ||
<goals> | ||
<goal>copy-dependencies</goal> |
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 seeing errors from the eclipse plugin on an initial build that go away on subsequent builds, presumably because the dependencies have been copied over. Is there a way for the plugin to declare its dependencies directly? If not, WDYT of using a script to set up the eclipse module's deps instead of doing it with maven?
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 saw that behaviour before. A full maven clean install
should work fine, as can be seen at the Travis and AppVeyor CI runs.
I thought about using a script as well, but a) Maven already knows about the dependencies (and where to copy them from) and b) Maven should do w/o the manual copy step. Somehow.
Any Maven expert here to rescue?
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.
A full
maven clean install
should work fine
So it does, thanks. In that case in think this is fine for now.
<parent> | ||
<groupId>com.google.googlejavaformat</groupId> | ||
<artifactId>google-java-format-parent</artifactId> | ||
<version>1.3-SNAPSHOT</version> |
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.
@cushon Do you know a way to retrieve the "current" version of interest automatically? Using properties? It feels strange to me to repeat here and in the dependencies
section below (and other configuration files in this PR) when all I want to express is "use the version of my parent pom".
<dependency> | ||
<groupId>com.google.googlejavaformat</groupId> | ||
<artifactId>google-java-format</artifactId> | ||
<version>1.3-SNAPSHOT</version> |
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.
${project.version}
?
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.
Resolves to locally defined version 0.0.1
.
Having a global property like:
<properties>
<googlejavaformat.version>1.3-SNAPSHOT</googlejavaformat.version>
</properties>
seemed to help, but:
[WARNING]
[WARNING] Some problems were encountered while building the effective model for com.google.googlejavaformat:google-java-format:jar:1.3-SNAPSHOT
[WARNING] 'version' contains an expression but should be a constant. @ com.google.googlejavaformat:google-java-format-parent:${googlejavaformat.version}, ...\sormuras\google-java-format\pom.xml, line 39, column 12
[WARNING]
[WARNING] Some problems were encountered while building the effective model for com.google.googlejavaformat:google-java-format-eclipse-plugin:eclipse-plugin:1.0-${format.version}
[WARNING] 'version' contains an expression but should be a constant. @ line 27, column 12
[WARNING] 'version' contains an expression but should be a constant. @ com.google.googlejavaformat:google-java-format-parent:${googlejavaformat.version}, ...\sormuras\google-java-format\pom.xml, line 39, column 12
[WARNING]
[WARNING] Some problems were encountered while building the effective model for com.google.googlejavaformat:google-java-format-parent:pom:1.3-SNAPSHOT
[WARNING] 'version' contains an expression but should be a constant. @ com.google.googlejavaformat:google-java-format-parent:${googlejavaformat.version}, ...\sormuras\google-java-format\pom.xml, line 39, column 12
[WARNING]
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING]
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING]
Gonna leave it constant.
Upgrade to version 1.3 which is the version in which the Eclipse plugin was included [1]. [1] google/google-java-format#118 Change-Id: Ica5f3bcef13ebfa57ad97d706ab10c4fc5d3fa30
Upgrade to version 1.3 which is the version in which the Eclipse plugin was included [1]. The only change in formatting caused by this version is removal of spaces after <li> tags in javadoc comments [2]. [1] google/google-java-format#118 [2] google/google-java-format@bbb742e Change-Id: I91ad790b586c9ea7e9af1b61016ea4276b90576d
Now that upstream released Eclipse plugin for google-java-formatter code formatter: [1], document how to set it up and running. [1] google/google-java-format#118 Contributed-By: Liam Miller-Cushon <cushon@google.com> Contributed-By: Christian Stein <sormuras@gmail.com> Change-Id: I48c6fc8b37f2fc7f5236fcdbe2f429f2396943be
See #52 for details.
State
Update to 1.3-SNAPSHOT and use SnippetFormatter
Some rough edges remain: