-
Notifications
You must be signed in to change notification settings - Fork 378
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
first design stab at API refactor #39
Conversation
@mheffner if you have any feedback on the API design, I'd super appreciate it :) |
Hmm maybe the underlying semantics are better conveyed with an API like
|
#include "restclient-cpp/restclient.h" | ||
|
||
// initialize RestClient | ||
RestClient::init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would this do in the per-connection case? Could this be handled inside Restclient in a call_once
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to investigate this further, but this is what libcurl says about the global functions:
It isn't actually required that the functions be called at the beginning and end of the program -- that's just usually the easiest way to do it. It is required that the functions be called when no other thread in the program is running.
And if I want to retain the simple declaration that it's safe to use the connection object from within threads, then I can't do the call_once
in there as I can't guarantee there aren't any other threads running. I think for the simple API this is fair and makes it a lot nicer to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sounds good. I think it's fine to keep then.
this adds the new proposed API to the header files and also splits them out into distinct files.
this also consolidates unit tests into a test file per source file
this properly gets rid of the connection object in the static API methods
this would previously always set the ret.code to the curl response code, even if the query failed. Maybe we want to switch to that in the future, but for now we return -1 for invalid queries still.
this adds a timeout test as the first unit test
this implements the method to return an Info struct from the connection object. It contains the configuration of the object as well as metrics about the last request.
ok this should be the first draft implementation of the new API. I punted on the request builder functionality as I think repeating something like |
this splits up the way to interact with endpoints into a simple and an
advanced usage way. The simple way mostly corresponds to the old API. The new
way is via a
connection
object that can be configured with differentproperties.
related to #27, #31, #33