make Child::try_wait return io::Result<Option<ExitStatus>> #39512
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
I probably should've written r? @alexcrichton, since this is coming from #38903. |
|
Looks good to me, thanks @oconnor663! @rust-lang/libs any thoughts about this change? |
|
Seems to be inconsistent with how pretty much every other non-blocking operation is handled. |
|
@nagisa can you elaborate some more? I think a good point is made that What other blocking/nonblocking APIs do we have? |
|
|
|
[added fixes for the try-wait.rs tests, and rebased] The API currently in master tries to be consistent with pub enum TryWaitError {
Io(std::io::Error),
WouldBlock,
}So we might end up with some inconsistency either way? Though maybe that My goal here was to make the non-blocking case as nice as possible, by making it a good option to just do |
|
Oh and is this the right thing to do to run that test locally right now? I found that in https://github.com/rust-lang/rust/blob/master/src/bootstrap/README.md, and it seems to work. |
|
@oconnor663 looks like the doctest may be failing? |
|
Ok I'm gonna go ahead an r+, but we can also revisit this signature before stabilization! @bors: r+ |
|
📌 Commit bd57742 has been approved by |
|
This fails on Windows https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1843/job/4ysk609ppr5hxfm2 @bors r- |
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this
if let Some(status) = foo.try_wait()? {
...
} else {
...
}
instead of this
match foo.try_wait() {
Ok(status) => {
...
}
Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
...
}
Err(err) => return Err(err),
}
The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.
Tracking issue: rust-lang#38903
|
Rebased and fixed the Windows break (hopefully). |
|
@bors: r+ |
|
📌 Commit 2a345bb has been approved by |
make Child::try_wait return io::Result<Option<ExitStatus>>
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this
if let Some(status) = foo.try_wait()? {
...
} else {
...
}
instead of this
match foo.try_wait() {
Ok(status) => {
...
}
Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
...
}
Err(err) => return Err(err),
}
The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.
Tracking issue: rust-lang#38903
make Child::try_wait return io::Result<Option<ExitStatus>>
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this
if let Some(status) = foo.try_wait()? {
...
} else {
...
}
instead of this
match foo.try_wait() {
Ok(status) => {
...
}
Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
...
}
Err(err) => return Err(err),
}
The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.
Tracking issue: rust-lang#38903
make Child::try_wait return io::Result<Option<ExitStatus>>
This is much nicer for callers who want to short-circuit real I/O errors
with `?`, because they can write this
if let Some(status) = foo.try_wait()? {
...
} else {
...
}
instead of this
match foo.try_wait() {
Ok(status) => {
...
}
Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
...
}
Err(err) => return Err(err),
}
The original design of `try_wait` was patterned after the `Read` and
`Write` traits, which support both blocking and non-blocking
implementations in a single API. But since `try_wait` is never blocking,
it makes sense to optimize for the non-blocking case.
Tracking issue: rust-lang#38903
This is much nicer for callers who want to short-circuit real I/O errors
with
?, because they can write thisinstead of this
The original design of
try_waitwas patterned after theReadandWritetraits, which support both blocking and non-blockingimplementations in a single API. But since
try_waitis never blocking,it makes sense to optimize for the non-blocking case.
Tracking issue: #38903