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): Alter naming convention for consistency #3282

Merged
merged 18 commits into from
Oct 21, 2023

Conversation

xyjixyjixyji
Copy link
Contributor

We remove blocking for consistency and will add async support in the future, where we will add async keyword to the API.

This PR also rename the opendal_reader_read() first argument from self to reader, since self is not a C convention.

@xyjixyjixyji
Copy link
Contributor Author

Relate #3281

@xyjixyjixyji
Copy link
Contributor Author

Oh, I notice this probably could break the swift, go and zig as well, @Xuanwo . Would you please take a look at that when you got time?

@jiaoew1991
Copy link
Contributor

Oh, I notice this probably could break the swift, go and zig as well, @Xuanwo . Would you please take a look at that when you got time?

BTW c binding is used by other language bindings, does it ready to release? 🤔

@xyjixyjixyji
Copy link
Contributor Author

BTW c binding is used by other language bindings, does it ready to release? 🤔

Hmm I think the major functionalities are already there, list, read, write. But there are still many other functionalities that we do not support.

There are a couple of things to consider

  1. Does C binding always stay sync with other bindings? => For this question I think the answer is no. I would say other bindings that depend on C binding release with a certain version of C binding.
  2. Do we need to align all functionalities that are implemented by other released bindings?

If the answer to the second question is yes, we may need to start figuring out what we are missing.
If the answer is no, i.e. we implement them in the future release, I think the C binding is ready for a release.

In any way this needs some discussions, but it is good to mention this issue up since we do need to make a release soon 🤣

@Xuanwo Xuanwo changed the title chore(bindings/C): Remove blocking from API names for consistency refactor(bindings/C): Remove blocking from API names for consistency Oct 14, 2023
@Xuanwo
Copy link
Member

Xuanwo commented Oct 14, 2023

Does C binding always stay sync with other bindings?

For now, YES. For always, I don't know either. It depends on the future development of OpenDAL.

Pin exact tags of C binding for other binding is complex and makes the development work much harder.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

I'm willing to help address the issues for zig/go/swift. However, I would prefer to address the following comments first:

bindings/c/include/opendal.h Outdated Show resolved Hide resolved
bindings/c/include/opendal.h Outdated Show resolved Hide resolved
bindings/c/include/opendal.h Outdated Show resolved Hide resolved
bindings/c/include/opendal.h Outdated Show resolved Hide resolved
bindings/c/include/opendal.h Outdated Show resolved Hide resolved
bindings/c/include/opendal.h Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Oct 14, 2023

This is a breaking change, it's better to record it in C binding's upgrade.md.

@xyjixyjixyji
Copy link
Contributor Author

xyjixyjixyji commented Oct 14, 2023

This is a breaking change, it's better to record it in C binding's upgrade.md.

I will address these issues later today. Thanks for the review.

@jiaoew1991
Copy link
Contributor

Looking forward to the API stabilizing, so that I can continue with the integration work of Milvus. 😄

@Xuanwo
Copy link
Member

Xuanwo commented Oct 19, 2023

Looking forward to the API stabilizing, so that I can continue with the integration work of Milvus. 😄

I believe this PR will address most of the API flavor issues.

@xyjixyjixyji
Copy link
Contributor Author

This seems to be a PR that is related to many things, I will mark this as draft and make this ready for review after some polishments.

@xyjixyjixyji xyjixyjixyji marked this pull request as draft October 19, 2023 04:20
@Xuanwo
Copy link
Member

Xuanwo commented Oct 20, 2023

cc @Ji-Xinyou, would you like to take a review? Thanks!

Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo marked this pull request as ready for review October 20, 2023 10:37
@Xuanwo Xuanwo requested a review from PsiACE as a code owner October 20, 2023 10:37
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Copy link
Member

Xuanwo commented Oct 20, 2023

cc @jiaoew1991, I have added an upgrade.md docs for C binding, would you like to take a look?

https://github.com/apache/incubator-opendal/blob/8396c4398af9281cbcbc4258e8773b05e85e75cf/bindings/c/upgrade.md

Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo changed the title refactor(bindings/C): Remove blocking from API names for consistency refactor(bindings/C): Alter naming convention for consistency Oct 20, 2023
Signed-off-by: Xuanwo <[email protected]>
@xyjixyjixyji
Copy link
Contributor Author

cc @jiaoew1991, I have added an upgrade.md docs for C binding, would you like to take a look?

https://github.com/apache/incubator-opendal/blob/8396c4398af9281cbcbc4258e8773b05e85e75cf/bindings/c/upgrade.md

Added commits look perfect to me.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 21, 2023

cc @Ji-Xinyou, do you think this PR is good to go?

Signed-off-by: Xuanwo <[email protected]>
@xyjixyjixyji
Copy link
Contributor Author

cc @Ji-Xinyou, do you think this PR is good to go?

Yes, I think this PR is ready :)

@Xuanwo Xuanwo merged commit c7bfe23 into apache:main Oct 21, 2023
31 checks passed
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.

3 participants