Only parse valid json

[iBicha]

Closes #12

[iBicha]

cc @TwitchBronBron

[TwitchBronBron]

Thanks for your effort on this. Your changes are a breaking change to the library that I don't think we should implement in this way. There are many servers that send JSON but don't return the correct content-type, and there are also multiple content-types that could contain valid json.

I think a non-breaking implementation would be to add an option to the Requests_response function, such as parseJson: boolean. It should be true by default, but you could also make it a string and support auto if you're not always sure what response you're going to get.

[iBicha]

More than happy to do it that way. I've updated the PR.

Although I have to say I really disagree with the approach. Setting a flag to NOT parse json is counter intuitive, and kinda against the design of every requests lib in other languages.

Most languages (fetch api in js, requests in python, etc) with have an explicit json() method on the response that will trigger the parsing.
I understand that this is a breaking change, but since roku-requests is at version 0.2.0, which means it has not committed to a stable API yet, I think it should be acceptable (better yet encouraged) to do the breaking change for the sake of better design. At least if we're following semantic versioning.

To justify what I did on my first attempt: I knew there will be concerns for breaking changes, that's why I opted for checking the content-type. In my opinion, any server returning invalid content type is non-compliant and it should be treated as a bug (and still, these are pretty rare anyway) so my attempt was to only parse valid json in most cases. But even on this first attempt, I still think it's better to rip the bandaid off and make the breaking change. The impact is so minimal anyway.

Users will have to change code from

r = Requests().get("https://api.github.com/events")`
?r.json

to

r = Requests().get("https://api.github.com/events")`
?r.json()

Which is very minimal effort for a way better experience.

[TwitchBronBron]

I completely agree with your assessment. However, we inherited/rescued this project from another developer that wasn't able to maintain it anymore. The ropm published version 0.2.0 was because I didn't want to make it 1.0.0 until I figured out if the ropm version was functional. Then, I completely forgot to bump the version to 1.0.0. This library has been around for ages, and as such, I should go ahead and bump the version to 1.0.0 so that's clear.

If you're interested, we could start the process of working towards a v2 release, but I'd like to make sure we clearly thought through additional breaking changes so we could group them together.

[iBicha]

@TwitchBronBron I'm fine with that, we can merge this as is, and figure out v2 as we go. Thanks!

[TwitchBronBron]

Great! I just kicked off the v1.0.0 release. Then I'll merge this and cut a 1.1.0 release. Thanks!

[TwitchBronBron]

Can you update the readme to include the new option?

[TwitchBronBron]

Looks great. Thank you so much for your work on this!

[iBicha]

Looks great. Thank you so much for your work on this!

My pleasure, I also updated changelog!

[TwitchBronBron]

roku-requests@1.1.0 has this feature.