Json parse failure handling

[bhornseth]

Trying to wrap up the few outstanding v2 changes and I need some design input on this one

I can't remember if we have an internal ticket tracking this and didn't find one when I looked briefly.

questions:

• is it worth having a SparkApi (gem) exception specifically for this, or should we close Improve handling of non-JSON responses #139 as Won't Fix and keep the behavior how it's always been? The main benefit I see in adding a custom exception is we can have more actionable error messages in the logs, even if we don't catch the exceptions.
• if we do move forward with this, should the error instance include the request metadata? I could see it being nice to have this error outside the SparkApi::ClientError hierarchy so handlers that catch that generic exception continue to behave the same and don't start picking up this new exception as well, but I'm curious to hear other thoughts on that.

[wlmcewen]

👋

[wlmcewen]

I think I'm OK with keeping the dependencies pretty wide open, but you may want to be tighter on dev dependencies. This is of course assuming we don't checking a lock file.

I don't have a clear solution here, but there is a definite problem with working on a gem because one's local versions are vastly different from the main author.

[wlmcewen]

Perhaps a guideline here is to limit to >= an exact version that is known to work.

[bhornseth]

If I understand you correctly, you're talking about the problem that happens when your local copy has preinstalled gems, and the tests all pass, but the from-scratch job on Travis or our build server fails, either when installing deps or because a changed dependency caused a test to fail?

[wlmcewen]

Yup, exactly. Everything works fine up until a new version of the dependency is released, and trashes your next build. It's a nuisance, and can be downright maddening to figure out the last good list of deps.

[wlmcewen]

As to your questions, I very much think it's worthwhile handling these errors in the gem, and present a more appropriate message. The multijson response is rather terrible.

Including the request and response information as a part of the error is something I've wanted to do for a long time, hell, even providing a clean manner to log this information for successful requests wouldn't be bad, so long as overhead was manageable, or at least the feature was configurable.

[bhornseth]

Apologies: I unintentionally made this PR against master, not the v2 base that was intended. I've updated it and it's a much smaller review now 🤣

[wlmcewen]

MUCH SMALLER

[bhornseth]

if you want to review the v2 changes, those are over here 👉 #153 😄