Skip to content

Remove some usage of unwrap() #2072

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

Merged
merged 2 commits into from
Jan 4, 2020

Conversation

jtgeibel
Copy link
Member

This commit reduces the usage of unwrap within the request/response
lifecycle, and documents a few remaining cases. Usage of unwrap in
tests, binaries, or server boot code is generally okay and was not
reviewed.

Remaining work:

  • Some usage of unwrap in git.rs and src/tasks/ background jobs.
    This code is not directly in the request/response lifecycle, but
    should be reviewed.
  • Current usage in middleware seems okay, but could be documented.
    In most cases, such as with the handler in AroundMiddleware, the
    conduit API ensures a handler is set.

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

This commit reduces the usage of unwrap within the request/response
lifecycle, and documents a few remaining cases.  Usage of unwrap in
tests, binaries, or server boot code is generally okay and was not
reviewed.

Remaining work:

* Some usage of unwrap in `git.rs` and `srcc/tasks/` background jobs.
   This code is not directly in the request/response lifecycle, but
  should be reviewed.
* Current usage in middleware seems okay, but could be documented.
  In most cases, such as with the handler in `AroundMiddleware`, the
  conduit API ensures a handler is set.
@jtgeibel jtgeibel force-pushed the fix/remove-some-unwraps branch from 14a1859 to 7aee526 Compare December 26, 2019 20:40
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Just one little thought about implementing std::error::Error. I'm ok merging without it tho!

@jtgeibel
Copy link
Member Author

jtgeibel commented Jan 4, 2020

I've push a commit to add impls for Debug and std::error::Error. It felt like I was missing something. I took a look at #2032 and it looks like I implemented these traits for the error type introduced there, so I think it makes sense to add it here as well, even if not currently needed in the codebase.

I wasn't expecting a conversion to the 2018 edition in this PR, but it's not very much, so I'll allow it ;)

Yeah, looks like it was missed before and I has already removed the extern crates. Thanks for letting it slide this time!

@bors r=carols10cents

@bors
Copy link
Contributor

bors commented Jan 4, 2020

📌 Commit a318cb3 has been approved by carols10cents

@bors
Copy link
Contributor

bors commented Jan 4, 2020

⌛ Testing commit a318cb3 with merge 5a08887...

bors added a commit that referenced this pull request Jan 4, 2020
Remove some usage of `unwrap()`

This commit reduces the usage of unwrap within the request/response
lifecycle, and documents a few remaining cases.  Usage of unwrap in
tests, binaries, or server boot code is generally okay and was not
reviewed.

Remaining work:

* Some usage of unwrap in `git.rs` and `src/tasks/` background jobs.
   This code is not directly in the request/response lifecycle, but
  should be reviewed.
* Current usage in middleware seems okay, but could be documented.
  In most cases, such as with the handler in `AroundMiddleware`, the
  conduit API ensures a handler is set.
@bors
Copy link
Contributor

bors commented Jan 4, 2020

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 5a08887 to master...

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.

5 participants