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 doc examples for most untrusted::Input methods. #2

Closed
wants to merge 2 commits into from

Conversation

frewsxcv
Copy link
Contributor

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@frewsxcv
Copy link
Contributor Author

I like documentation when they include examples and so does this RFC. @briansmith No feelings hurt if you feel otherwise. I couldn't think of a great example for read_all_mut and I'm a bit short on time so I didn't include one there. Let me know your thoughts and ideas for improvement.

@briansmith
Copy link
Owner

Thanks for this.

First, I think that your examples would be great to have as minimal unit tests. So, I suggest moving them to tests instead.

One of the problems that untrusted::Input tries to solve is DoS caused by panics. So, while I totally understand what you're trying to do here, I don't think that these code samples are actually good models for people to follow, because assert!() and assert_eq!() panic. Do you see what I mean here? (Sorry it's probably not very clear.)

So, instead, I think it would be better for the documentation to do similar stuff, but without using panicing stuff. e.g.

if a.is_empty() {
    return Err(());
}

Again, thanks for submitting this.

@frewsxcv
Copy link
Contributor Author

Extracted out the examples into unit tests: #4

@frewsxcv
Copy link
Contributor Author

I got rid of the panics. Not sure what you'll think of the changes. I'm pretty neutral.

@briansmith
Copy link
Owner

Sorry I never got around to looking at this. I'm just going to close this now to clear the backlog. I do think we should have some better documentation but I think we should refactor the API first.

@briansmith briansmith closed this Mar 16, 2019
@briansmith
Copy link
Owner

Also, thanks for the PR!

@frewsxcv frewsxcv deleted the doc-examples branch March 16, 2019 21:26
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.

2 participants