Error cleanup #117
Error cleanup #117
Conversation
eozturk1
commented
Dec 16, 2021
eozturk1
commented
- Error names and descriptions updated
- Unused or duplicate errors removed
- Errors reused when needed
facebook-github-bot
added
the
CLA Signed
akd/src/append_only_zks.rs
Outdated
| let (next_node_label, _) = hash_q | ||
| .pop() | ||
| .ok_or(AzksError::PopFromEmptyPriorityQueue(self.latest_epoch))?; | ||
| let (next_node_label, _) = hash_q.pop().unwrap(); |
There was a problem hiding this comment.
unwrap() is going to panic if it fails, rather than bubble up the error in an expected format. i.e. panic will terminate the app completely
There was a problem hiding this comment.
Also @Jasleen1 do we need the priority queue in the end here? Dropping it would let us also drop a crate dependency :)
EDIT yes we do, resolved.
There was a problem hiding this comment.
PopFromEmptyPriorityQueue implies the queue is empty but there is already an isEmpty check right before.
Could unwrap fail here?
There was a problem hiding this comment.
So maybe we could just rewrite as a while let Ok((next_node_label, _)) = ... statement, an avoid the is empty check all together.
There was a problem hiding this comment.
But yes I believe it can't fail, there's just better syntax to be used I think
There was a problem hiding this comment.
I like the while let approach, updated!
| /// Error propogation | ||
| DirectoryErr(DirectoryError), | ||
| /// Error propogation | ||
| /// Error propagation |
There was a problem hiding this comment.
I feel like the comments, which are exported in the crate publicly could be cleaned up too over "error propogation"
There was a problem hiding this comment.
Are they not auto-synced? I can look into it if not.
There was a problem hiding this comment.
I believe they're hand typed... no? Maybe there's a rust feature I'm unaware of? Check with intellisense maybe and see what it says for a description on that case.
There was a problem hiding this comment.
cargo doc generates the docs and puts them into target/doc . We can specify a website that serves these docs. (See [documentation](https://doc.rust-lang.org/cargo/reference/manifest.html#the-documentation-field)).
Alternatively, if we don't, docs.rs automatically builds the docs and serves them. So doc updates in the code should be reflected in the published repo.
