-
Notifications
You must be signed in to change notification settings - Fork 96
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
refactor(c/driver_manager): differentiate errors from driver manager #1662
Conversation
bbff6c1
to
e6387b9
Compare
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.
Thanks!
Would it be appropriate to add tests for at least a few of these cases?
if (!error) return; | ||
if (error->release && error->release != ReleaseError) { |
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.
Is it important to be able to reuse an existing error here that was previously initialized by the driver manager? It seems easier to explain to just always initialize a new one (or maybe it is worth explaining in a comment here?)
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 was preserving the behavior of just appending to an existing error (which admittedly is confusing - I can just get rid of that!)
@@ -696,6 +749,7 @@ AdbcStatusCode AdbcDatabaseGetOption(struct AdbcDatabase* database, const char* | |||
} else { | |||
const auto it = args->options.find(key); | |||
if (it == args->options.end()) { | |||
SetError(error, "Option not found"); |
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.
Is is easy to pass on here which option was not found?
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.
Done
@@ -948,6 +1006,7 @@ AdbcStatusCode AdbcDatabaseRelease(struct AdbcDatabase* database, | |||
AdbcStatusCode AdbcConnectionCancel(struct AdbcConnection* connection, | |||
struct AdbcError* error) { | |||
if (!connection->private_driver) { | |||
SetError(error, "AdbcConnectionCancel: must AdbcConnectionNew first"); |
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.
SetError(error, "AdbcConnectionCancel: must AdbcConnectionNew first"); | |
SetError(error, "AdbcConnectionCancel: must call AdbcConnectionNew first"); |
(...and a few below)
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 wonder if this diff would be suppressed if it were marked as linguist-generated in .gitattributes (it looks like it is currently linguist-vendored).
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 tweak it (though I think it wouldn't take effect until the next PR from what I recall seeing of how GitHub behaves)
Fixes #1653.