Skip to content
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

Move to MultiJson for jruby compatibility #1

Closed
wants to merge 1 commit into from
Closed

Move to MultiJson for jruby compatibility #1

wants to merge 1 commit into from

Conversation

colonel-sanders
Copy link

Despite the lead author's performance concerns, Oj isn't available for all interpreters. Proposing a switch to MultiJson to keep this gem broadly compatible.

@colonel-sanders
Copy link
Author

Upon reflection, considering a different approach

@xentek
Copy link
Owner

xentek commented Aug 12, 2014

Other than the use of mode: :compat with MultiJson (since I'm not 100% sure MultiJson.dump can take options, nor would do anything with the Oj option passed as is), I would welcome this pull request.

We are currently using MRI but considering a move to rbx and/or jruby, so would need to update our gems to use MultiJson anyway. Thanks for the patch in either case, even if you recalled it :)

@colonel-sanders
Copy link
Author

MultiJson should pass along options hashes (at least in more recent versions) to the underlying implementation. Aside from that, I'm no longer convinced that MultiJson is the best implementation choice here given its questionable future. Perhaps gson.rb under jruby and Oj under all C-extension-compatible platforms?

However, that's not the main problem with my PR.

More significantly, the code doesn't work under jruby. The jruby ext for do-postgres attempts to bind the json column values as regular varchars. Postgres steadfastly refuses to make the implicit conversion when it's explicitly bound this way.

I'll come back around with a new PR as soon as I get the code actually working properly.

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