-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add useOptionalForGetters option for Java 8 java.util.Optional on getters #344
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
Conversation
and well - can you think of a way to implement this without breaking java 7 compatibility? :-) |
I think there are ways :) I haven't had a chance to look through this yet,
|
This PR is incomplete and stale. I'm closing this as I don't believe it's going to be completed, but we can use these changes for reference in the future. |
@joelittlejohn I would really appreciate this feature and would be willing to work the get it included. What did you feel was missing from the earlier pull request? |
@EitZei Do you have some thoughts about how to implement this without breaking Jav 7 compatibility? |
Maybe we should cut a 1.0 version of this library for Java 8 and drop Java 7 support going forward. There are a load of things we would do differently in the generated code if we were target Java 8 and later I think. Many modern Java constructs can't be supported by CodeModel though (the library we use to create .java files) so maybe we should also try to move to something else. I've recently found https://github.com/square/javapoet but I'm also not sure how well this library supports either Java 7 or Java 8 constructs. |
@EitZei If you really do need this feature (and soon), you should consider building your own version of jsonschema2pojo that has this PR merged. |
To my limited understanding this PR is already Java 7 compatible after the https://github.com/joelittlejohn/jsonschema2pojo/pull/344/files/8b5a566c514d81f7976747db6468904265cd3e96..HEAD amendment which makes the dependency to |
Ha, you're right. I actually didn't notice that update to the PR. |
Given this information would you be willing to merge this PR? |
Yes, I think this can be merged. I might change the config property name here but other than that I think this can be included in the next release. |
There's a bit more required here. This currently only supports the older |
Nice to get a visit from the past. I completely forgot how the traversal works, but I would either
which one do you think is better? |
check whether property is in the list of required properties
fd1d002
to
be3d35c
Compare
@joelittlejohn I copied the approach from NotRequiredRule to just loop through the required list. I did not include this in ScalaGen, is your goal to have feature parity? |
@joelittlejohn let's not make the same mistake twice :) when do you think you will have a chance to look at it? |
oh no @joelittlejohn it's happening again :) is there anything I can do to help you merge this? |
Let's do this 👍 |
Awesome work @holsky and @joelittlejohn! Any plans on making a new release? |
@EitZei the awesome work is all @holsky 😄 Another major change that has been merged recently is the move away from commons-lang to 'native' Java implementations of toString, equals and hashCode. I think I'll release an alpha/beta soon because I'm slightly concerned that some of the recent changes may have some unexpected consequences. |
This is just a draft - I would like to get some comments from you on the initial direction. Would it work this way or am I on the wrong track?
And is there a good reason why there are no tests for PropertyRule? (If not I'd implement a few happy path tests).
Fixes #342