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

Further improve bindings.rs documentation #75

Merged
merged 8 commits into from Oct 16, 2022
Merged

Further improve bindings.rs documentation #75

merged 8 commits into from Oct 16, 2022

Conversation

Techie-Pi
Copy link
Member

@Techie-Pi Techie-Pi commented Oct 9, 2022

This adds a "bin-dependency" to ctru-sys to doxygen-rs, a project I've been working on to improve Doxygen to Rustdoc conversion with ctru-rs in mind: Techie-Pi/doxygen-rs .

Now, every single Doxygen tag used by the bindings is translated (except a few markdown-like tags like p).

Example of the new documentation (also check #75 (comment)):
imagen

Maybe adding a dev-dependency is not desired, in that case I could send PRs only with the improvements to bindings.rs, though it would not be easy to automate or to replicate on other machines.

Related: #32 #64 rust-lang/rust-bindgen#955

This adds a "_bin-dependency_" on ``ctru-sys`` to doxygen-rs, a project I've been roking on to improve Doxygen to Rustdoc conversion: https://github.com/Techie-Pi/doxygen-rs
@Techie-Pi Techie-Pi marked this pull request as draft October 9, 2022 12:54
@Techie-Pi Techie-Pi marked this pull request as ready for review October 9, 2022 12:59
@Techie-Pi
Copy link
Member Author

CI fails because the changes done in #74 have not been back-ported here

@Techie-Pi Techie-Pi marked this pull request as draft October 12, 2022 19:53
This commit adds support for translating:
- ``retval``
- ``return``
- ``sa``
- ``see``
- ``code``
- ``endcode``
- ``warning``
- ``ref``
- ``note``
- ``name``

There are no remaining tags in `ctru-sys` to translate
@Techie-Pi
Copy link
Member Author

This last commit adds support for the tags that remained untranslated. Further examples of tags that are now translated

Return values

imagen

Clickable references

imagen

Clickable links

imagen

@Techie-Pi Techie-Pi marked this pull request as ready for review October 12, 2022 21:42
@Techie-Pi
Copy link
Member Author

(#75 (comment))

@ian-h-chamberlain
Copy link
Member

(Github won't let me comment directly on the file, it seems)

As a minor nit, the spacing for some of the #[doc] attributes seems a bit odd:

#[doc = "Undefined syscall."]
#[doc = ""]

pub const EXCEVENT_UNDEFINED_SYSCALL: ExceptionEventType = 8;
#[doc = "Reasons for an exception event."]
#[doc = ""]

pub type ExceptionEventType = ::libc::c_uint;

I would expect more like

#[doc = "Undefined syscall."]
#[doc = ""]
pub const EXCEVENT_UNDEFINED_SYSCALL: ExceptionEventType = 8;
// maybe no newline here, idk
#[doc = "Reasons for an exception event."]
#[doc = ""]
pub type ExceptionEventType = ::libc::c_uint;

I don't imagine people will read bindings.rs directly very often, so probably not a big deal in general.

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

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

Looks good! I am trying to fix my local toolchain installation to build the docs locally and check them out but your screenshots look like a nice improvement over what was there.

ctru-sys/Cargo.toml Outdated Show resolved Hide resolved
@Techie-Pi
Copy link
Member Author

Looks good! I am trying to fix my local toolchain installation to build the docs locally and check them out but your screenshots look like a nice improvement over what was there.

Try deleting the Cargo.lock file on root, that did it for me

@ian-h-chamberlain
Copy link
Member

ian-h-chamberlain commented Oct 15, 2022

Try deleting the Cargo.lock file on root, that did it for me

Thanks, it was more of an OS-level issue but I got it solved and the docs look good!

Actually on a semi-related note... I wonder if we ought to check in a Cargo.lock for the sake of CI, and maybe add another CI job to the matrix that deletes Cargo.lock before running just to find breakage from upstream. Just a thought

I do see some weird rendering for docstrings on constants, but I'm guessing that's a rustdoc bug and nothing to do with your changes:

example screenshot

Screen Shot 2022-10-15 at 2 26 22 PM

Clicking into them seems to work fine so idk what's up with that.

@Techie-Pi
Copy link
Member Author

I do see some weird rendering for docstrings on constants, but I'm guessing that's a rustdoc bug and nothing to do with your changes:

Yeah, that's an issue with Rustdoc, it also happened before #64

Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

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

Looks great! I would still refer to the original docs, but it doesn't hurt to have them all here as well.

ctru-sys/bindgen.sh Outdated Show resolved Hide resolved
@Techie-Pi Techie-Pi marked this pull request as draft October 16, 2022 15:57
@Techie-Pi
Copy link
Member Author

Wait a second, I think I need to check one thing

@Techie-Pi Techie-Pi marked this pull request as ready for review October 16, 2022 15:58
@Techie-Pi
Copy link
Member Author

Techie-Pi commented Oct 16, 2022

Alright, done. I thought I had committed something I shouldn't, but that isn't the case

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

4 participants