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

Remove .ok() calls on Results #152

Open
rnbguy opened this issue Jan 26, 2024 · 0 comments
Open

Remove .ok() calls on Results #152

rnbguy opened this issue Jan 26, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@rnbguy
Copy link
Member

rnbguy commented Jan 26, 2024

There are a few Result::ok() calls in the source code - a few of them are redundant, and a few of them are used wrongly.

  1. We convert Option to Result and then to Option. This is redundant as we can simply use the first Option.

fn lookup_module(&self, port_id: &PortId) -> Option<ModuleId> {
self.port_to_module_map
.get(port_id)
.ok_or(RouterError::UnknownPort {
port_id: port_id.clone(),
})
.cloned()
.ok()
}

  1. We ignore the fact path serialization has failed.

let path: Option<Path> = request.path.try_into().ok();
let modules = self.modules.read_access();
let height = Height::from(request.height as u64);
for IdentifiedModule { id, module } in modules.iter() {
match module.query(&request.data, path.as_ref(), height, request.prove) {

  1. We ignore a codec encode/decode that has failed in concrete Codec implementations. For example, with JsonCodec,

fn encode(d: &Self::Value) -> Option<Self::Encoded> {
serde_json::to_string(d).ok()
}
fn decode(bytes: &[u8]) -> Option<Self::Value> {
let json_string = String::from_utf8(bytes.to_vec()).ok()?;
serde_json::from_str(&json_string).ok()
}

The Codec trait should have an associated type Error which lets us return a Result with the failure type.

@rnbguy rnbguy added the bug Something isn't working label Jan 26, 2024
@rnbguy rnbguy changed the title Beware of .ok() calls on Result Remove .ok() calls on Results Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant