-
Notifications
You must be signed in to change notification settings - Fork 56
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
Feat:recovereable error functions in scope src/cli #1106
Feat:recovereable error functions in scope src/cli #1106
Conversation
Hi, just to offer some directions in terms of prioritization for a first round of work (this doesn't need to be resolved on a single PR). These are the places that contain the most important parts of the codebase, as far as I can tell:
So starting on those is a better first step 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing what @arthurpaulino said, it'll probably be easier if this is done over multiple PRs since there are many places in the codebase that show up in the cargo clippy
output.
I'm adding a couple of comments below to try and clarify some points, let me know if you have any questions!
src/z_data/serde/de.rs
Outdated
let a: [u8; 2] = x.clone().try_into().unwrap(); | ||
let x_clone = x.clone(); | ||
let a: [u8; 2] = [x_clone[0], x_clone[1]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't modify how the code works: if x_clone[0]
or x_clone[1]
is out of bounds, there will be a runtime panic.
An alternative to prevent the panic is to add a match
block to handle the result of x.clone().try_into()
, and return an Err
if it is not Ok
. (Similar to the code inside deserialize_u32
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I misunderstood the "match guards" in Rust.
I will fix it soon!
src/z_data/serde/de.rs
Outdated
// This error can't happend? | ||
Err(_) => Err(SerdeError::Type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error could still happen if x.len()
is not 8
, in which case the .try_into()
would fail to convert it into a [u8; 8]
.
This is because in your change you removed the if x.len() == 8
condition in the match arm (before the =>
, see the match guards section of the rust reference for a thorough explanation).
Previously, in both this function (and the deserialize_u32
function that has a similar pattern), it would never run the x.clone().try_into().unwrap();
line unless the if x.len() == 8
condition passed. This means that even though there was an .unwrap()
here, it would never panic in practice, since the condition guarantees that the .try_into()
should always succeed.
cargo clippy
has no way of checking the above conditions, and that's why it listed this location among the rest. (Unfortunately, there's no easy way of checking this besides looking at each .unwrap()
call in a case-by-case basis.)
Thank @arthurpaulino and @wwared for useful suggestions. I will check it after Lunar New Year. |
d55306a
to
1049fbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A stylistic remark for the incoming changes of this kind.
I'd usually state this change as a nitpick, but since you're going to make several changes of this kind, it's worth to start with a style that better suits lurk-rs
from the beginning.
Also, thank you!
If this is all for what you found in src/cli
, it's fine for an initial PR. The other folders can be covered on other PRs
Thank you for your comments, @arthurpaulino. It's would be better if I followed the code style you recommend! |
60a5f2a
to
d2c0e5f
Compare
The current changes look good. I will wait until you set the PR as ready for review before I do another check and run the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
ffb4071
* Create a (new) InterpretationData to hold interpretation data on MultiFrame * Remove some unwraps/expects because interpretation data is no longer spread across multiple holders * [unrelated] Undo a change from #1106 where a panic was the desired behavior
* Create a (new) InterpretationData to hold interpretation data on MultiFrame * Remove some unwraps/expects because interpretation data is no longer spread across multiple holders * [unrelated] Undo a change from #1106 where a panic was the desired behavior
Motivation
More context
If this PR look good, I will continue update for another scope like @arthurpaulino suggest in this comment
Thank you for all review and suggestions!