-
Notifications
You must be signed in to change notification settings - Fork 123
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
Added bijection-avro module. Updated bijection-guava to support various encodings #129
Conversation
* @tparam T compiled Avro record | ||
* @return Injection | ||
*/ | ||
def toJson[T <: GenericRecord](schema: Schema): Injection[T, Array[Byte]] = { |
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.
Should this not be [T, String]
? Json is always a string 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.
Hi Oscar,
The problem with using [T,String] is that I would have to convert Array[Byte] to a String and back myself and that opens the whole issue of Charset encoding. Should I use UTF-8, platform's default charset or let the user specify the Charset encoding? By passing Array[Byte] I was letting the client deal with byte serializing and deserializing issue. If you have any suggestion on how to deal with Charset issue for both apply and invert, I would be more than happy to switch to [T,String]
Thanks. Looks good. Just some minor comment about prefering Json injection going to string. |
Do you have any comments regarding the Guava change? |
I have switched Json Injection to [T,String] using UTF-8 charset since thats what we are using throughout the project |
Looks good to me. About Guava, I don't use it much, but it looks fine. I wonder, do you need the upgrade? Would it still compile and pass if we stuck at 13? My concern is people winding up with a higher version without intending due to the dependency on bijection-guava (some of our repos are still on old versions). |
@johnynek Had to upgrade Guava because all the encoding function were introduced in Guava 14.0. If there are backward compatibility concerns, then you dont have merge that in. http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/io/BaseEncoding.html |
nah, screw it. Looks good to me. :) |
Added bijection-avro module. Updated bijection-guava to support various encodings
Added Avro Module which provides following injections
Updated bijection-avro to Guava 14.0. Added bijections for Base16, Base32, Base32HEX and Base64. I know there is already a Base64 bijection in Core but I would appreciate it if we leave the Gauva based Base64 bijection in bijection-gauva.
Let me know if you need me to change anything