Add Error implementation for std::sync::mpsc::RecvTimeoutError.#37527
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
Stebalien
left a comment
Stebalien
left a comment
There was a problem hiding this comment.
FYI, implementations of stable traits are always stable so you might as well just mark it so.
src/libstd/sync/mpsc/mod.rs
Outdated
There was a problem hiding this comment.
I'd say something like "timed out waiting on channel". The current message doesn't actually say that this is a timeout.
src/libstd/sync/mpsc/mod.rs
Outdated
src/libstd/sync/mpsc/mod.rs
Outdated
There was a problem hiding this comment.
You're also going to need a Display implementation.
Mark-Simulacrum
force-pushed
the
mpsc-recvtimeouterror-error-impl
branch
from
0aa39b4 to
63d2fc4
Compare
|
Fixed review comments. |
src/libstd/sync/mpsc/mod.rs
Outdated
There was a problem hiding this comment.
Can't this be reduced now to self.description().fmt(f)? Though, I would think you'd want to write something like RecvTimeoutError(<message here>) instead of the bare message.
I notice that both of these comments also apply to std::sync::mpsc::{RecvError, TryRecvError} so maybe they are wrong.
There was a problem hiding this comment.
Yeah, I'm not sure either. That does seem reasonable; I'm happy to change all three to this format if that's what needs to be done.
Mark-Simulacrum
force-pushed
the
mpsc-recvtimeouterror-error-impl
branch
from
63d2fc4 to
79d18d0
Compare
src/libstd/sync/mpsc/mod.rs
Outdated
There was a problem hiding this comment.
These will need to be "1.14.0".
Mark-Simulacrum
force-pushed
the
mpsc-recvtimeouterror-error-impl
branch
from
79d18d0 to
2af6111
Compare
alexcrichton
added
the
T-libs-api
|
Looks great to me, thanks @Mark-Simulacrum! @rfcbot fcp merge |
| #[stable(feature = "mpsc_recv_timeout_error", since = "1.14.0")] | ||
| impl fmt::Display for RecvTimeoutError { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| match *self { |
There was a problem hiding this comment.
You could write this entire function as self.description().fmt(f).
There was a problem hiding this comment.
@durka mentioned this here: #37527 (comment), but since the other implementations for display also duplicate this code, I think it's best to leave this as-is for now. I'll be happy to file another PR (once this lands) deduplicating this code.
|
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @alexcrichton, I wasn't able to add the |
|
@bors: r+ Thanks! |
|
📌 Commit 2af6111 has been approved by |
…or-error-impl, r=alexcrichton Add Error implementation for std::sync::mpsc::RecvTimeoutError. Fixes rust-lang#37525.
…or-error-impl, r=alexcrichton Add Error implementation for std::sync::mpsc::RecvTimeoutError. Fixes rust-lang#37525.
Rollup of 30 pull requests - Successful merges: #37190, #37368, #37481, #37503, #37527, #37535, #37551, #37584, #37600, #37613, #37615, #37659, #37662, #37669, #37682, #37688, #37690, #37692, #37693, #37694, #37695, #37696, #37698, #37699, #37705, #37708, #37709, #37716, #37724, #37727 - Failed merges: #37640, #37689, #37717
9 participants
Fixes #37525.