search

heroku / heroku.rb

DEPRECATED! Official Heroku Ruby Legacy API wrapper
161 stars 41 forks source link

drop okjson in favor of multi_json #73

Closed geemus closed 10 years ago

jkakar commented 10 years ago

This looks good to me, +1! Do we normally update changelog.txt or is that something we only do when we cut a release?

geemus commented 10 years ago

Historically I've just waited and updated changelog at release time. Open to discussing if that should change.

jkakar commented 10 years ago

No need to change, I just wanted to understand the process, thanks for explaining.

geemus commented 10 years ago

Sure thing. That is more or less the process I use on everything that I manage releases for and it seems to work well-enough.

wuputah commented 10 years ago

:-1:. MultiJson should not be a dependency. There' s a number of easy alternatives, such as: https://github.com/heroku/heroku-bouncer/blob/master/lib/heroku/bouncer/json_parser.rb

(This particular code above does not deal with error handling, but it wouldn't be hard to add that.)

If nothing else, the version constraint needs to be relaxed (~>1.8.2 is much too strict). I'd propose ~>1.5.

geemus commented 10 years ago

What is wrong with multijson? Also, I guess I'm concerned that the difficulty of getting other jsons installed inside the toolbelt environment might be bad. Anyway, would be helpful to have a deeper understanding of where you are coming from here. Thanks!

wuputah commented 10 years ago

For any gem, adding a dependency is a big deal - you're asking everyone everywhere to add that gem. The heroku-api gem is still the easiest way to use the Heroku API in your Ruby app, so it isn't just for the toolbelt. For instance, I use it in Help app. So it should only be if necessary. And MultiJson is, IMO, never necessary.

MultiJson (and Ruby+Json in general) is actually a huge problem, and is actually completely deprecated now -- it's use in the Ruby community is no longer recommended, and will be removed in Rails 4.1. I can't make an official recommendation on what to do at this time for the toolbelt, but I'd recommend talking to Terence. Read this, for a start: https://github.com/intridea/multi_json/pull/138#issuecomment-24468223

For me, I've gone to some length to make it so that MultiJson doesn't actually get loaded into a project. You'll note that I use the lowest possible version number here (1.5.x) for compatibility with other gems that have a MultiJson dependency (I've since shipped some bug fixes, so its 1.5.3). Maybe that's dumb, but basically I initially used 0.0.1, and then had to bump it to 1.5.0 to get it to satisfy a dependency. In any case, unless heroku-api needs 1.8.x for some reason, you should really relax the dependency.

wuputah commented 10 years ago

(Edits and such are above, should you be reading your email instead of github.com. I always edit a lot. Including this. Sorry.)

geemus commented 10 years ago

I was reading the email. UG.

@hone - advice?

hone commented 10 years ago

@geemus what's the motivation here? performance? why use many different adaptors?

geemus commented 10 years ago

@hone - I believe there are multiple issues. There are some encoding issues related to non-ascii values that okjson chokes on, so we'd like to improve that situation (especially in toolbelt). On the heroku.rb side, in addition to that, we want to not get in the way/conflict with folks if they want to have other json stuff in their app that includes heroku.rb (and allow for better performance to be opted-in to). So partly performance, partly encoding and partly flexibility despite conflicting monkey patches.