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

docs(bindings/C): The documentation for OpenDAL C binding #2373

Merged
merged 25 commits into from
May 31, 2023

Conversation

xyjixyjixyji
Copy link
Contributor

This PR uses doxygen to generate documentations for C binding.

Developers could call make doc to generate documentations in ./docs/doxygen/html/index.html.

The documentations in the Rust code for C binding now uses the doxygen style.

@xyjixyjixyji
Copy link
Contributor Author

Related #2321

This is relative a big PR but almost w/o funcationality changes.
Reviewers could ignore the additions under ./docs and try make doc locally instead if wanted.

@xyjixyjixyji
Copy link
Contributor Author

It seems the CI is failing on the files generated by doxygen, is there any workaround?

@suyanhanx
Copy link
Member

suyanhanx commented May 30, 2023

You may not commit them. We just need to generate those docs in the CI job and deploy them as a part of our site.
Please take our docs ci job config as an example. @Ji-Xinyou

FYI:

@xyjixyjixyji
Copy link
Contributor Author

You may not commit them. We just need to generate those docs in the CI job and deploy them as a part of our site.

Sure

bindings/c/docs/doxygen/html/annotated.html Outdated Show resolved Hide resolved
bindings/c/Doxyfile Show resolved Hide resolved
@xyjixyjixyji
Copy link
Contributor Author

I have updated on the github action, I am not an expert on it so PTAL ❤️

@xyjixyjixyji
Copy link
Contributor Author

xyjixyjixyji commented May 30, 2023

Another problem is that the licence header CI requires C files to be started with

/**
 * sth
 */

However, that would make the header part of the documentation (in the first struct appears in the doc). Therefore I change it to

/*
 * sth
 */

But this cannot pass the licence header check.

@suyanhanx
Copy link
Member

Another problem is that the licence header CI requires C files to be started with

/**
 * sth
 */

However, that would make the header part of the documentation (in the first struct appears in the doc). Therefore I change it to

/*
 * sth
 */

But this cannot pass the licence header check.

cc @tisonkun

@tisonkun
Copy link
Member

tisonkun commented May 31, 2023

However, that would make the header part of the documentation (in the first struct appears in the doc). Therefore I change it to

I know for this part. Perhaps I can change the default in the hawkeye core later. For now, you can add the following config to the licenserc.toml file to change it:

[mapping.SLASHSTAR_STYLE]
extensions = ["c", "h"]

For changes upstream, perhaps something like this one korandoru/hawkeye@33e482f

@jiaoew1991
Copy link
Contributor

https://github.com/apache/incubator-opendal/blob/59af64663692ac5109e50f47de7c77a91a551c2a/bindings/c/src/lib.rs#L91-L96

I have a simple question, why does this place still support memory scheme? Other schemes should also have similar methods, which should be easy to add.

@xyjixyjixyji
Copy link
Contributor Author

I have a simple question, why does this place still support memory scheme? Other schemes should also have similar methods, which should be easy to add.

Thanks for pointing it out! 👍 This is indeed a problem.

@Xuanwo
Copy link
Member

Xuanwo commented May 31, 2023

I have a simple question, why does this place still support memory scheme? Other schemes should also have similar methods, which should be easy to add.

Yes, we can remove generate_operator.

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.

Thanks a lot!

@Xuanwo Xuanwo merged commit 1adbb54 into apache:main May 31, 2023
@Xuanwo Xuanwo mentioned this pull request Jun 2, 2023
@suyanhanx suyanhanx mentioned this pull request Jun 6, 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.

5 participants