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

Default to json parser #37

Closed
wants to merge 3 commits into from
Closed

Default to json parser #37

wants to merge 3 commits into from

Conversation

sethherr
Copy link

@sethherr sethherr commented Dec 15, 2017

The standard lib json parser is good enough - and this definitely shouldn't be a required parameter.

And then, it swallows the error? Doesn't make sense to me. Frankly, to me, this entire bit should be removed and you should just parse with the standard lib if you get a string (ruby < 1.9 can figure it out themselves - particularly if there was an error thrown - oh wait, you require 1.9.3 so that doesn't even matter).

But in the interest of not making a major modification - it's already checking the type, why raise an error if it's a string? Why not just make life better and just work?

The standard lib json parser is [good enough](http://www.mikeperham.com/2016/02/09/kill-your-dependencies/) - and this definitely shouldn't be a required parameter. I was stunned when it raised an error.
@teeparham
Copy link
Member

We should remove the json_parser option altogether & always use the stdlib. And add a test for the example that failed.

@sethherr
Copy link
Author

I mean, that makes sense to me - but that would be a breaking change. This doesn't break anything and seems like it could just be merged without a problem.

I could open a PR to remove the json_parser option after this if you'd like.

@teeparham
Copy link
Member

What do you think about #38 instead?

@sethherr
Copy link
Author

Looks good to me!

It will require a version update, but it's definitely a better answer. Closing this 🙂

@sethherr sethherr closed this Dec 16, 2017
@sethherr sethherr deleted the patch-2 branch December 16, 2017 18:13
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