Convert certain methods from Option type to Result type #14
Conversation
|
Hi @artech-git, thanks for the changes! I think these are looking good so far and address some of our TODO's. Could you update the PR with these tweaks?
Once that done we will take another look, but it looks good so far. Also, can you explain why the |
I was actually trying to experiment with |
| version = "1.0.69" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "5d727cae5b39d21da60fa540906919ad737832fe0b1c165da3a34d6548c849d6" | ||
| checksum = "134c189feb4956b20f6f547d2cf727d4c0fe06722b20a0eec87ed445a97f92da" |
There was a problem hiding this comment.
Are the dependency updates in Cargo.lock necessary? We like to keep dependency updates separate from other changes and only add if needed.
There was a problem hiding this comment.
Alright noted I believed since I applied some dependencies updates on my local Envoirment, we can remove that .
| // pub for tests | ||
| pub fn decrypt_in_place(&mut self, key: &Key, salt: &Salt) -> Option<()> { | ||
| pub fn decrypt_in_place(&mut self, key: &Key, salt: &Salt) -> Result<(), PacketError> { |
There was a problem hiding this comment.
I am seeing a warning when running tests. For clippy, did you run this? It should catch them all:
cargo clippy --all-features --all-targets -- -D warnings
| let incoming = ControlPacket::parse_and_decrypt_in_place( | ||
| encrypted, | ||
| &self.decrypt.rtcp.key, | ||
| &self.decrypt.rtcp.salt, | ||
| )?; | ||
| ) | ||
| .ok_or(EndpointError::RtcpSenderSsrcNotSet)?; |
There was a problem hiding this comment.
I think parse_and_decrypt_in_place should return EndpointError? They are all in the same file and hence should be able to share Error definitions. You could add all the different kinds of RTCP errors that the function might return. That might increase the scope of the PR a lot. Hmm, is it worth it?
Maybe just make the error more generic, like RtcpError. Because errors are more than just "Sender SSRC was not set", right?
There was a problem hiding this comment.
I think making the error type generic is a nice idea I initially thought about it though, but didn't proceeded with that due to the over complexity it might present in maintaining it in the long run hence I opted for writing errors enum for each struct possible and keeping minimum only.
Also maybe we can add a another module for specifically for handling scenarios only( all errors seperated in that module only).
Again we need some more brainstorming on what can work out best and avoiding the burden, but speaking of generic errors I can happily work on this if you can present with some rough idea of what errors and the possible variants it should have for the scenarios !
just to explain what I understood is written below:
#[derive(...)]
pub enum RTXErrors {...
}
#[derive(...)]
pub enum SsrcErrors{...
}
#[derive(...)]
pub enum PacketErrors {...
}
#[derive(...)]
pub enum EndpointError<T> {...to visually explain it, here is a small diagram I have prepared
There was a problem hiding this comment.
I am not sure that many error types is necessary. For example, I only see one RTXError, so I think it can just stay as an EndpointError. For SsrcError, that looks okay.
artech-git
changed the title
|
The PR seems to be looking better. Here would be a checklist to complete this PR:
Then we can do a final review and hopefully close this soon. Thanks! |
Got it, thanks for the inputs. I will work on it asap. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been closed due to inactivity. |
For improved error handling certain methods of codebase required
Resulttype for appropriate error handling, instead ofOptiontypeThese changes for supporting error handling are still very experimental, and therefore require multiple inputs and guidance for refinement and improvements 😊, feel free to let me know.