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

Added doc-comments to Nox #13

Merged
merged 23 commits into from May 21, 2024
Merged

Conversation

nestordemeure
Copy link
Contributor

Following the discussion in issue 11 and the preliminary test in PR 12, here is a PR adding doc comments to the entire Nox codebase. As previously stated, the comments are AI-generated, with the goal of speeding up the documentation of the library (it is easier to improve a bad comment than to write one from scratch).

The PR is best examined by generating the documentation (cargo doc --open --no-deps --all-features).
Do not hesitate to comment and criticize; some comments will definitely require manual improvements.

One thing that is still missing, and should probably be done by hand, is a lib.rs top comment that serves as a documentation readme (ideally with a small code example1 showcasing usage and/or a link to the relevant section of the Elodin doc). You can find an example of one such comment here (rendered there).

Footnotes

  1. Overall, code examples are the big thing still missing from a new user's perspective.

Copy link
Contributor

@akhilles akhilles left a comment

Choose a reason for hiding this comment

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

Thanks for preparing this PR! I did an initial pass over most of the files, just have a few nitpicks. Generally, the comments are pretty good.

libs/nox/src/client.rs Outdated Show resolved Hide resolved
libs/nox/src/client.rs Outdated Show resolved Hide resolved
libs/nox/src/client.rs Outdated Show resolved Hide resolved
libs/nox/src/client.rs Outdated Show resolved Hide resolved
libs/nox/src/comp.rs Outdated Show resolved Hide resolved
libs/nox/src/spatial.rs Outdated Show resolved Hide resolved
libs/nox/src/spatial.rs Outdated Show resolved Hide resolved
libs/nox/src/spatial.rs Outdated Show resolved Hide resolved
libs/nox/src/spatial.rs Outdated Show resolved Hide resolved
libs/nox/src/tensor.rs Outdated Show resolved Hide resolved
@nestordemeure
Copy link
Contributor Author

Thank you! I have just fixed the comments following your feedback.

Copy link
Contributor

@akhilles akhilles left a comment

Choose a reason for hiding this comment

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

Have a few more suggestions, but otherwise lgtm!

libs/nox/src/comp_fn.rs Outdated Show resolved Hide resolved
libs/nox/src/exec.rs Outdated Show resolved Hide resolved
libs/nox/src/matrix.rs Outdated Show resolved Hide resolved
libs/nox/src/scalar.rs Outdated Show resolved Hide resolved
libs/nox/src/matrix.rs Outdated Show resolved Hide resolved
libs/nox/src/param.rs Outdated Show resolved Hide resolved
libs/nox/src/scalar.rs Outdated Show resolved Hide resolved
@nestordemeure
Copy link
Contributor Author

Thank you! I included your comment in the latest commit.

Copy link
Contributor

@akhilles akhilles left a comment

Choose a reason for hiding this comment

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

I fixed the merge conflict.

@akhilles akhilles merged commit 5407533 into elodin-sys:main May 21, 2024
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.

None yet

2 participants