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

Remove the use of Crack and Json and instead use MultiJson #2

Closed
wants to merge 6 commits into from
Closed

Remove the use of Crack and Json and instead use MultiJson #2

wants to merge 6 commits into from

Conversation

joshk
Copy link
Contributor

@joshk joshk commented Mar 4, 2011

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

@maxthelion
Copy link

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

@joshk
Copy link
Contributor Author

joshk commented Mar 4, 2011

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

@joshk
Copy link
Contributor Author

joshk commented Mar 4, 2011

ohhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh

Well, I really should have picked up on that one :)

@joshk
Copy link
Contributor Author

joshk commented Mar 4, 2011

How to look like a fool in 3 simple comments :)

@maxthelion
Copy link

Don't worry, we don't expect everyone to get our silly in-jokes :)

@mloughran
Copy link
Contributor

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?

@joshk
Copy link
Contributor Author

joshk commented Mar 4, 2011

You can be as illogical as you like ;)

@joshk
Copy link
Contributor Author

joshk commented Mar 24, 2011

Hey Guys,

Any further feedback on this patch?

Thanks,

Josh

@joshk
Copy link
Contributor Author

joshk commented Apr 12, 2011

Y U NO LIKE MY PATCH

@sferik
Copy link

sferik commented May 4, 2011

👍 ❤️ this patch.

What's holding up merging this in and releasing a new version?

@mloughran
Copy link
Contributor

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 :)

@mloughran mloughran closed this May 17, 2011
jameshfisher pushed a commit that referenced this pull request Apr 21, 2018
Update old branch with latest commits from upstream
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.

4 participants