-
Notifications
You must be signed in to change notification settings - Fork 930
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
Adding error objects to request responses #61
Conversation
I don't know how to test all of them, unfortunately, so the coverage here is less than optimal. Help figuring this out would be very welcome.
Looks like here you're referring to |
Ah, I was using the system CURL, for which that value does exist. I suspect it can be safely elided, which I will do. |
Hrm. Currently the tests are failing because the test for |
… is dependent upon features selected at compile time…but then, there really isn't much that can be done about it.
Oh, I just meant conditional compilation by examining some macro definition. When I build using system curl on OSX, I get SSL for free. (And building using the included CURL fails, but that's a different issue). But when I build on Windows with the included CURL, there is no SSL (as you well know). So it doesn't make sense to test SSL functionality where it isn't available (but it seems like it should when it is). Not my favorite thing to do ever (because it requires us to talk to an implementation detail that oughtn't be exposed), but I'm going to just do runtime check against the feature flags in |
Just did a thorough read over your PR here. This is a great start! I'm going to build out some support inside the embedded test server to surface the other errors you're having trouble reproducing. Once I get a good hold of that, and get as close to 100% coverage again as possible, I'll merge this in and throw my changes on top of it. |
Great! Let me know if there is anything I can do from my end to ease that process. |
Oh hello, just wanted to check in and see if there was anything I could do to pitch in on this? |
Hey @DEGoodmanWilson, just been busy with work! I'm gonna cut 1.2.0 and just get it out there, then get this into the master branch and we'll take it from there. Expect this within the next few days. Thanks for your patience! |
Adding error objects to request responses
Sometimes CURL just gives up, and it would be nice to have some understanding of why. It would also be nice if we could do it in a non-CURL-centric way. This PR introduces a new Error object to encapsulate specifically internal and HTTP errors that can occur when performing a request. It also introduces a new enum ErrorCode to provide a machine-readable way of assessing errors that is also independent of the CURL codes.
Not all CURL errors map to ErrorCodes. This is because many of the errors are for protocols beyond the current scope of cpr (e.g. FTP). Many are due to errors on the server which are already suitably handled by the HTTP status_code feature. And a lot of them are CURL specific, of course, and so get rolled up into the generic INTERNAL_ERROR. (Also, many of the SSL errors are very specific, and I wasn't sure if the way CURL granulated them was both universally applicable and user friendly, so I just lumped them together under a small handful of SSL errors). The CURL-generated human readable text is placed in the ERROR object in case the end developer requires more context.
Test coverage is not 100%, because in many cases I am not certain how to induce certain of the CURL error conditions.
I've tried to stick to the house style, please let me know if there are any formatting mistakes as well! I look forward to reading your comments.