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

refactor(bindings/C): Implement error with error message #3250

Merged
merged 21 commits into from
Oct 13, 2023

Conversation

xyjixyjixyji
Copy link
Contributor

@xyjixyjixyji xyjixyjixyji commented Oct 9, 2023

  1. Implements the error with error code and error message

    • Since the error message is on heap, any non-null error needs to be freed by hand
  2. Implement opendal_result_operator_new(), now this function returns internal error message.

  3. Refactor the tests and examples to use the above implementations.

This PR seems large, but the essence of it is to add an error message into the opendal_error. Other changes are following this feature.

Close #3244

@xyjixyjixyji
Copy link
Contributor Author

\cc @jiaoew1991 Is this kind of error message expected for you? Feel free to raise any comment on this.

@xyjixyjixyji
Copy link
Contributor Author

xyjixyjixyji commented Oct 10, 2023

Note that the changes in this PR will break the CI of other bindings depending on C binding, i.e. Swift and Zig. (Go currently do not have a CI so it is fine, but it probably will break as well).

@xyjixyjixyji xyjixyjixyji changed the title refactor(error): Implement error with error message refactor(bindings/C): Implement error with error message Oct 10, 2023
bindings/c/examples/basicrw.c Outdated Show resolved Hide resolved
bindings/c/examples/error_msg.c Outdated Show resolved Hide resolved
bindings/c/examples/error_msg.c Outdated Show resolved Hide resolved
bindings/c/src/error.rs Show resolved Hide resolved
bindings/c/src/error.rs Outdated Show resolved Hide resolved
bindings/c/tests/error_msg.cpp Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Oct 10, 2023

Note that the changes in this PR will break the CI of other bindings depending on C binding, i.e. Swift and Zig.

It would be better for us to address them simultaneously in this PR. I can take a look at zig.

@xyjixyjixyji
Copy link
Contributor Author

\cc @Xuanwo , thanks for the review, the suggestions are good and are adopted accordingly.

@xyjixyjixyji xyjixyjixyji added enhancement New feature or request bindings/c labels Oct 10, 2023
@Xuanwo Xuanwo removed enhancement New feature or request bindings/c labels Oct 10, 2023
@Xuanwo
Copy link
Member

Xuanwo commented Oct 10, 2023

We don't need labels for PR.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 13, 2023

I have fixed zig support, the next is swift.

Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Copy link
Member

Xuanwo commented Oct 13, 2023

Go support also fixed, next one is swift.

Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo merged commit 9515e8b into apache:main Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bindings/C: we need seperate opendal_operator_new and opendal_operator_init
2 participants