Skip to content

Added support for javaType on integer and number properties. #287

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

Merged

Conversation

ctrimble
Copy link
Collaborator

This PR adds support for the javaType on integer and number properties.

integer supports the following javaType values:

  • int
  • java.lang.Integer
  • long
  • java.lang.Long

number supports the following javaType values:

  • float
  • java.lang.Float
  • double
  • java.lang.Double

When javaType is present, the configuration settings are ignored.

@ctrimble
Copy link
Collaborator Author

This relates to #223 and seems to be a regression of #41.

@joelittlejohn
Copy link
Owner

Ha, so it is. A bit of archeology needed :)

Thanks for submitting this! I'll give it a look over and merge tomorrow.

@ctrimble
Copy link
Collaborator Author

Thanks @joelittlejohn !

@joelittlejohn
Copy link
Owner

Took a quick look at #41, and no this is not a regression of that change. That change relates to being able to specify primitive types when you use the javaType property inside 'object'.

I'm interested to know what prompted you to make this change. Did you need to change the type of specific properties and not want to change the configuration globally with useLongIntegers/useDoubleNumbers/usePrimitives?

@ctrimble
Copy link
Collaborator Author

@joelittlejohn I needed most of my integer fields to be java.lang.Integer, but a few required java.lang.Long. We were double configuring the maven plugin to do this, but that was a lot of config to get things to work. This seems much more clear.

My current project is polyglot and we are using JSON Schema as a way of keeping our interfaces sane. I am starting to have a lot of schemas in my build, so fine grained control over things like is coming up more frequently.

@ctrimble
Copy link
Collaborator Author

Totally willing to work on this some, if you think the implementation needs to be different.

@joelittlejohn
Copy link
Owner

Hi Christian. Looks great to me. Lots of tests too so I'm perfectly happy to merge.

To be perfectly honest, I probably would have left this as an unconstrained option and simply used whatever type is found in javaType. E.g. If I want to use BigInteger/BigDecimal I could use that, or maybe my own custom type (as long as I give Jackson a serializer/deserializer for my custom type, it'll work). I'm happy to go with these constraints though as they make sense in most cases. If anyone feels limited by them, we can relax it in a later version.

@ctrimble
Copy link
Collaborator Author

Do you already have a method around that operates like Class.forName(String), but handles primitives like int, long, etc.? If so, I will take a moment and update the PR.

@joelittlejohn
Copy link
Owner

This class may help:

https://github.com/joelittlejohn/jsonschema2pojo/blob/master/jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/PrimitiveTypes.java

You could use a check like if (isPrimitive(javaType)) { return primitiveType(javaType); } else { return owner.ref(javaType); } or maybe try { return primitiveType(javaType); } catch (GenerationException e) { return owner.ref(javaType); }. Or something else along those lines :)

@ctrimble
Copy link
Collaborator Author

Cool. Let me take a look at this real quick and add a commit for it. If it looks good, we can squash. If it sucks, we can drop the commit. Would like this to be done right.

@ctrimble
Copy link
Collaborator Author

@joelittlejohn the implementation is generalized. Let me know what you think and I can clean up the history.

@joelittlejohn
Copy link
Owner

Looks good. Squash away and I'll merge tomorrow. Thanks again for all your contributions!

@ctrimble ctrimble force-pushed the feature_java-type-primatives branch from da28479 to ad1f56b Compare February 17, 2015 00:25
@ctrimble
Copy link
Collaborator Author

Squashed and ready to merge.

joelittlejohn added a commit that referenced this pull request Feb 17, 2015
Added support for javaType on integer and number properties.
@joelittlejohn joelittlejohn merged commit 598ed31 into joelittlejohn:master Feb 17, 2015
@joelittlejohn joelittlejohn added this to the 0.4.8 milestone Feb 20, 2015
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.

2 participants