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

docs: updating use ? & Result instead of unwrap #860

Merged
merged 1 commit into from Jan 20, 2019
Merged

docs: updating use ? & Result instead of unwrap #860

merged 1 commit into from Jan 20, 2019

Conversation

ralph-mcteggart
Copy link
Contributor

@ralph-mcteggart ralph-mcteggart commented Jan 20, 2019

Following on from #657

Fixing doc test to use ? with Result instead of unwrap() just to (hopefully) show better practice.

Motivation

@liranringel had mentioned that the examples needed updating but that doc tests were still to do. I checked all the remaining tests and this was the only one left that could do with a change, albeit a minor improvement.

Solution

Simply changing unwrap() to use ? operator and change function signature to return Result which would be more idiomatic for someone coming to copy this code for their own use.

This hopefully lets us close out #657

@ralph-mcteggart ralph-mcteggart changed the title docs: updating unpack to use Result instead of unpack docs: updating unwrap to use Result instead of unpack Jan 20, 2019
@ralph-mcteggart
Copy link
Contributor Author

ralph-mcteggart commented Jan 20, 2019

Just realised I wrote unpack in commit message 🙄

EDIT: Pushing amended commit message

@ralph-mcteggart ralph-mcteggart changed the title docs: updating unwrap to use Result instead of unpack docs: updating unwrap to use Result instead of unwrap Jan 20, 2019
@ralph-mcteggart ralph-mcteggart changed the title docs: updating unwrap to use Result instead of unwrap docs: updating use ? & Result instead of unwrap Jan 20, 2019
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@tobz tobz merged commit 91f20e3 into tokio-rs:master Jan 20, 2019
@ralph-mcteggart
Copy link
Contributor Author

ralph-mcteggart commented Jan 20, 2019

@tobz thanks for the quick look! What's normal practice from here in this repo to close out the PR and the corresponding issue? Just up to one of you guys?

EDIT: saw you merged 👍

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

2 participants