Add IPC Transport #424
Add IPC Transport #424
Conversation
src/transports/ipc.rs
Outdated
| }; | ||
|
|
||
| for output in outputs { | ||
| let id = match &output { |
There was a problem hiding this comment.
This could simply be let id = output.id().clone()?
src/transports/ipc.rs
Outdated
| } | ||
| } | ||
| } | ||
| Err(err) => log::warn!("Got bad IPC response bytes: {:?}", err), |
There was a problem hiding this comment.
Should it really be a warn? What if we simply didn't get a full response yet (but it does have } or ])?
src/transports/ipc.rs
Outdated
| let value = match helpers::to_result_from_output(output) { | ||
| Ok(value) => value, | ||
| Err(err) => { | ||
| log::warn!("Unable to parse output into rpc::Value: {:?}", err); | ||
| continue; | ||
| }, |
There was a problem hiding this comment.
If you extract to a function this would become
| let value = match helpers::to_result_from_output(output) { | |
| Ok(value) => value, | |
| Err(err) => { | |
| log::warn!("Unable to parse output into rpc::Value: {:?}", err); | |
| continue; | |
| }, | |
| let value = helpers::to_result_from_output(output) | |
| .map_err(|err| log::warn!("Unable to parse output into rpc::Value: {:?}", err))?; |
which imho looks much nicer
src/transports/ipc.rs
Outdated
| }, | ||
| Some(Err(err)) => { | ||
| log::error!("IPC read error: {:?}", err); | ||
| break; |
There was a problem hiding this comment.
Should this be return Err(...) instead?
src/transports/ipc.rs
Outdated
| log::error!("IPC read error: {:?}", err); | ||
| break; | ||
| }, | ||
| None => break, |
There was a problem hiding this comment.
I'd change this to return Ok(()) and make the whole loop the last statement in the function.
src/transports/ipc.rs
Outdated
|
|
||
| loop { | ||
| tokio::select! { | ||
| message = messages_rx.next() => match message { |
There was a problem hiding this comment.
The match here could be if let Some(message) = message {...}
FelipeRosa
force-pushed
the
add-ipc-transport
branch
from
a57a6bf
to
05b0b5e
Compare
|
@tomusdrw Thanks for the review! We're finishing up with the doc and tests, along with adding all the cfg flags. Then we'll take a look at your comments (and remove the draft flag). Be right back! |
Co-authored-by: lsunsi <sunsi.lucas@gmail.com>
FelipeRosa
force-pushed
the
add-ipc-transport
branch
from
05b0b5e
to
1788be6
Compare
|
@tomusdrw Done deal! All comments replied. We sent fixups in order to make your re-review easier, but we'll squash all commits into one if everything is fine. Just let us know. |
Regarding this issue: #415
This PR does not close it yet, because it doesn't add the duplex and batch transports.
But it's a start.