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

Add docstrings for a few small modules! #286

Merged
merged 2 commits into from
May 18, 2021

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented May 1, 2021

Problem

This is the first in a set of planned PRs to add inline documentation to the Rust API which can then be beautifully published with cargo doc. See #285.

I am not a Signal engineer. I am making my best guesses as to the appropriate docstring content. I think I've been able to do just fine, but please push back if I'm mischaracterizing anything -- it's easy for me to just revert anything that seems possibly too bikesheddable.

Solution

  1. Add module docs for the libsignal_protocol crate.
  2. Add docs to the device_transfer crate.
  3. Add docs to the address module.

Result

We will have established a rapport for further improvements to the documentation and module layout as per #285!

Copy link
Contributor

@jrose-signal jrose-signal left a comment

Choose a reason for hiding this comment

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

Okay, here we go! Some initial comments for this effort, which I suppose is a monotonic improvement over no documentation. :-) Thanks for stepping up!

rust/protocol/src/address.rs Show resolved Hide resolved
rust/protocol/src/address.rs Outdated Show resolved Hide resolved
rust/protocol/src/lib.rs Outdated Show resolved Hide resolved
rust/protocol/src/address.rs Outdated Show resolved Hide resolved
rust/protocol/src/address.rs Outdated Show resolved Hide resolved
@Be-ing
Copy link

Be-ing commented May 4, 2021

Thanks for working on this! GitHub is showing a warning about the email you've used for these commits:
image
I suggest to fix your Git configuration so your commit email matches the GPG key you're signing these commits with then rebase.

@cosmicexplorer
Copy link
Contributor Author

@Be-ing thanks! I have fixed the issue.

Copy link
Contributor

@jrose-signal jrose-signal left a comment

Choose a reason for hiding this comment

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

Getting there!

rust/protocol/src/address.rs Outdated Show resolved Hide resolved
rust/protocol/src/address.rs Outdated Show resolved Hide resolved
@cosmicexplorer cosmicexplorer force-pushed the docs-1 branch 4 times, most recently from 4df8902 to a1ea3af Compare May 5, 2021 03:53
@cosmicexplorer cosmicexplorer force-pushed the docs-1 branch 2 times, most recently from 01eebe8 to 9872e21 Compare May 5, 2021 04:53
@cosmicexplorer cosmicexplorer force-pushed the docs-1 branch 2 times, most recently from 9a476fa to 3206468 Compare May 8, 2021 14:23
- Add module docs for the `libsignal_protocol` crate!
- Add docs to the `device-transfer` crate!
- Add docs to `address`!

beef up the docs for `address` because this is an important struct!

respond to review comments

revert module visibility changes

de-clutter the docstring for `.device_id()`

add missing docs warning

fix uuid deps

add doctest for `ProtocolAddress::new()`!

avoid pulling in uuid into dev-dependencies
@jrose-signal
Copy link
Contributor

jrose-signal commented May 10, 2021

Ah, right, some doc comments will make it into the generated C header for Swift. You can run swift/build-ffi --generate-ffi to make sure that's up to date.

@cosmicexplorer
Copy link
Contributor Author

Added the generated swift ffi changes!

@jrose-signal jrose-signal merged commit 2e7c36b into signalapp:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants