-
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
Remove the use of Crack and Json and instead use MultiJson #2
Conversation
…lso removes the Json gem dependency and leaves it up to the user of the gem. A test had to be removed as the error was not thrown with all Json decoders.
…s method ourselves.
Hi Josh, Thanks for this. We get quite a few comments from people who actually like the fact that Pusher depends on crack... ;) Will leave it one of the other guys to review these changes. Max |
Hey Max, People like that it depends on Crack? But all Crack is used for is the Hash#to_params. For our apps at work I prefer not to have dependencies which are not used, especially when I use much faster Json parsing libs like Yajl. In fact, I think a better solution is to check if Hash#to_query is defined (which ActiveSupport adds) and use that, otherwise add Hash#to_query (rename to_params to to_query). I am interested to know what the others think :) Thanks, Josh |
ohhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh Well, I really should have picked up on that one :) |
How to look like a fool in 3 simple comments :) |
Don't worry, we don't expect everyone to get our silly in-jokes :) |
Thanks for this Josh. I hadn't seen the MultiJson library before - makes a lot of sense. I'll also be sad to see the end of 'crack' - must we be always logical? |
You can be as illogical as you like ;) |
Hey Guys, Any further feedback on this patch? Thanks, Josh |
👍 ❤️ this patch. What's holding up merging this in and releasing a new version? |
Sorry about the rather long delay.... I've now got off my backside and merged this patch - thanks! I kept the crack dependency because I like the joke, the whole crack gem is almost no code anyway, we're only requiring the bit we need, and it's my prerogative :) |
Update old branch with latest commits from upstream
Hey Guys,
Mixed into here are some smaller additions like ignoring the Gemfile.lock and suppressing logger output during one test. But the main reason for the pull request is the removal of the Json and Crack gem dependencies and the addition of MultiJson.
Crack was only being used for one Hash core ext, so instead of bring in an entire lib when only one method is needed, I have copied the method over instead.
Also, instead of specifying the Json gem as a dependency, instead I have used MultiJson. The big advantage is if a user is using a different Json gem (yajl, json-pure, active support json etc.) then MultiJson will use the fastest one available. I did have to remove one test as not all Json gems raise errors when invalid content is being passed in to be parsed, but generally I think this is a big win.
If you have any questions or concerns please let me know,
Thanks,
Josh