Skip to content
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

tiledb-rs error styling part 1: Context::capi_call returns its own error type #191

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

rroelke
Copy link
Collaborator

@rroelke rroelke commented Nov 20, 2024

In tiledb-tables, we've begun to declare at least one Error enum per module. If a module foo calls a function from a module bar, then foo::Error has an enum variant approximating Bar(bar::Error). This can help build a causality chain in error output.

We have discussed bringing this error style into tiledb-rs, and this pull request is a foray into doing so. We begin with the core of tiledb-rs, Context::capi_call. This pull request adds CApiError which is returned by Context::capi_call. The rest of the changes fell out of that in some way.

There are a few // SAFETY comments for new unwraps. These are added in situations where we try to convert a value returned from tiledb into one of the tiledb-rs enums. The presupposition is that if libtiledb returned TILEDB_OK then it must have given a valid enum value. I am personally comfortable with this assumption (I believe I am aligned with this blog about unwrap) but I'm also fine changing it if requested - it would nudge the code in the direction of more Error variants.

Copy link
Collaborator

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had the one comment about the odd feature conditional compilation thinger.

tiledb/api/src/stats.rs Outdated Show resolved Hide resolved
@rroelke rroelke force-pushed the rr/thiserror-part-1-CApiError branch from 2dd0e6d to bc6a57e Compare December 11, 2024 21:10
@rroelke rroelke merged commit f9352fe into main Dec 12, 2024
3 checks passed
@rroelke rroelke deleted the rr/thiserror-part-1-CApiError branch December 12, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants