-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Specify target JVM version #454
Specify target JVM version #454
Conversation
@joelittlejohn this change adds a new configuration option. The naming is strange for the Ant task and I am not very proficient at Gradle, so any notes around those items would be appreciated. |
069ee4e
to
9d0d1d4
Compare
@joelittlejohn I think the PR that adds the target option is ready to merge. |
Looks like I forgot setTarget( String target ) in the ant task and associated documentation. |
b5c029c
to
9f6fbee
Compare
@@ -65,7 +65,7 @@ public void antTaskDocumentationIncludesAllProperties() throws IntrospectionExce | |||
String documentation = FileUtils.readFileToString(new File("../jsonschema2pojo-ant/src/site/Jsonschema2PojoTask.html")); | |||
|
|||
for (Field f : Jsonschema2PojoTask.class.getDeclaredFields()) { | |||
assertThat(documentation, containsString(f.getName())); | |||
assertThat(documentation, containsString(">"+f.getName()+"<")); |
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.
Good spot 😄
Looking good to me. One thing, I think target here is a little confusing as it's easy to confuse this with the target directory. I prefer targetVersion everywhere, as you've used in the CLI. |
Other than that change, I can't see anything else to comment on. Nice work, I'll merge when you're ready. |
3c5af3d
to
08e3d12
Compare
@@ -131,6 +131,9 @@ | |||
|
|||
@Parameter(names = { "-da", "--disable-accessors" }, description = "Whether to omit getter/setter methods and create public fields instead.") | |||
private boolean disableAccessors = false; | |||
|
|||
@Parameter(names = { "-tv", "--target-version" }, description = "Target version for generated source files.") |
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 is the best way to describe this setting. Make all descriptions like this one.
08e3d12
to
233418e
Compare
@joelittlejohn |
Specify target JVM version
Exposes a
targetVersion
property to the rules and test compiler, specifying the target Java version to generate for.This fixes an issue during cross compilation where the source generated is valid for the JVM running the test suite but not for the JVM being targeted. For instance, auto boxing a cast like
(int)value
works inJava7
but not inJava6
.Interface Changes:
targetVersion
config property, with a default value ofmaven.compiler.target
.targetVersion
property, with a default value of1.6
targetVersion
property, with a default value of1.6
-tv
and--target-version
, with a default value of1.6
Limitations: This patch does not set the bootclasspath from
toolchains.xml
. So, there can still be issues whereJava7
classes and methods are referenced when generating code forJava6
. I attempted to add support using classes from Maven, but there is a classpath conflict with the testing forAnt
.-Xlint:-options
has been set during testing, to suppress warnings related to this.The changes here were taken from #386.