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

Improve binary documentation #330

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

JayKickliter
Copy link
Contributor

@JayKickliter JayKickliter commented Sep 10, 2020

For my colleagues and I, one of the hardest parts of coming up to speed with Rustler has been dealing with binaries. Even those of us with years of Rust experience were confused by the difference between Binary and OwnedBinary, despite the fact that Owned should say it all. I've tried my best to capture my confusion before I'm too familiar with codebase.

Included in this PR:

  • add module-level for the binary module, thus I had to remove the doc_hidden tag
  • When adding a # Panics section for OwnedBinary::from_unowned, I noticed that it could be simplified to the point that it's obvious that it will not panic (it calls copy_from_slice internally, which can panic, but won't since lengths will be checked before that call)
  • documents all binary api functions, even if trivially, for completeness sake

For reviewers: please run cargo doc --open and try all the intra-doc links in the binary module if possible. I've rustdoc to be very unreliable at warning on broken links.

@JayKickliter JayKickliter marked this pull request as draft September 10, 2020 22:46
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
@JayKickliter
Copy link
Contributor Author

@filmor thanks for all the feedback, I'll use all your suggestions. I figured I had factual errors, hence opening an early draft PR.

@JayKickliter JayKickliter force-pushed the jsk/add-documentation branch 2 times, most recently from ea6119f to 9f263e2 Compare September 16, 2020 23:47
@JayKickliter
Copy link
Contributor Author

I think I have addressed most/all of @filmor's comments, but may have introduced more factual errors in the process.

I've gotten a little busy lately, so I may not be able complete everything in the in checklist in the PR overview at the top, but I'll leave this PR as draft for little a longer.

@JayKickliter
Copy link
Contributor Author

I removed the draft tag as this PR is about as complete as I can make it. I've made made some changes since @filmor's initial review of the draft version, so I welcome any further comments.

Copy link
Member

@evnu evnu left a comment

Choose a reason for hiding this comment

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

Very nice, thanks! I only found two minor nits.

rustler/src/types/binary.rs Outdated Show resolved Hide resolved
rustler/src/types/binary.rs Outdated Show resolved Hide resolved
JayKickliter and others added 2 commits September 21, 2020 06:56
Co-authored-by: Magnus <evnu@posteo.de>
Co-authored-by: Magnus <evnu@posteo.de>
@evnu evnu merged commit 07ed913 into rusterlium:master Sep 21, 2020
@JayKickliter JayKickliter deleted the jsk/add-documentation branch September 21, 2020 16:25
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

3 participants