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

tracking issue for some Rust API learnability improvements #285

Closed
3 tasks
cosmicexplorer opened this issue May 1, 2021 · 3 comments
Closed
3 tasks

tracking issue for some Rust API learnability improvements #285

cosmicexplorer opened this issue May 1, 2021 · 3 comments

Comments

@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented May 1, 2021

Goal: Add docstrings and determine appropriate visibility specifiers for the Rust API

#152 describes a lack of API documentation. I found this to be a blocker when looking to depend on this crate from my own (also AGPLv3) rust project. There has clearly been a lot of extremely thoughtful work modularizing the jni, node, and ffi backends (that part was awesome!), and this is demonstrated in the much higher prevalence of docstrings in the libsignal_bridge crate than in the libsignal_protocol crate (compare the compiled docsites I've listed near the bottom of this post). I would like to help fix this.

I plan to do this mostly by adding docstrings to modules, structs, etc. Docstrings can be converted into a fantastic searchable webpage for API docs with cargo doc. To get an idea of what I'm thinking of, this is a github pages static site of the most recent docs build I have performed: https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/.

Additional orthogonal changes

However, in my perusal, I have also found a couple other types of changes I think will improve the API's learnability:

  1. Add (documented!) traits for standard behavior across structs. There are currently several methods spelled slightly differently and implemented as pub fn methods directly on structs, e.g. https://github.com/signalapp/libsignal-client/blob/87f205e80cc21ce0b1aa1a0e81f1a291d62fba49/rust/protocol/src/protocol.rs#L121-L123
  2. Prefer to return slices with a static size over arbitrary-sized &[u8] and Box<[u8]> -- this helps keep e.g. secret key data on the stack (vs Box<[u8]>) and avoids having to handle any lifetimes (vs &[u8]). See https://github.com/signalapp/libsignal-client/blob/87f205e80cc21ce0b1aa1a0e81f1a291d62fba49/rust/protocol/src/identity_key.rs#L30-L32

This issue previously described a desire to modify module paths as well in service of generated documentation, but that was discussed and decided against here: #286 (comment).

Progress

I'm currently working on this branch: https://github.com/cosmicexplorer/libsignal-client/tree/docs. At the time of writing, the diff contains 15 sequential commits, spanning a net +2492/-1671: https://github.com/signalapp/libsignal-client/compare/master...cosmicexplorer:docs?expand=1.

Nightly compiled docs.rs sites

These are github pages static sites of the most recent docs build I have performed (I push to gh-pages on my fork):

Individual PRs

I'm tracking the changes in order below as they are made into pull requests:

@cosmicexplorer
Copy link
Contributor Author

As per @jrose-signal in #286 (comment) I have removed the goal to change module visibility at all from this tracking issue. I do not think it is necessary at all for the goal of documentation, and I think this documentation work should ideally be in service of the existing module organization, not seeking to revise it unless there's some unforeseen issue.

@cosmicexplorer
Copy link
Contributor Author

Also as I noted in #286 (comment):

I think that cargo doc actually has flags to build things regardless of pub(crate) status, which I'm realizing is the only motivation to mess with the module structure at all.

So I'm hoping we can use pub(crate) or #[cfg(doc)] markers alone to accomplish this without disturbing much of the underlying code.

@cosmicexplorer
Copy link
Contributor Author

Closing this in favor of #467!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant