-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix factory-reset-app #30
Conversation
51e1667
to
4dbbf01
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.
This PR is still based on the superseded #29.
src/core_api.rs
Outdated
if locations.contains(&Location::Internal) | ||
|| locations.contains(&Location::External) => | ||
|| locations.contains(&Location::External) | ||
&& kinds.contains(ItemsToDelete::KEYS) => |
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.
I find this entire function very hard to read. I would suggest to extract one function that decides whether the object should be deleted, and one function that performs the actual deletion. Also it would be good to have a comment explaining the logic.
This condition evaluates to internal || (external && delete_keys)
if I read it correctly, but I think it should be (internal || external) && delete_keys
, no?
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.
Hmm, right, that condition was indeed incorrect.
I did not split the function fully like you recommended because that would require a lot of reference passing for the counters and everything, but I did simplify it significantly to make it clearer and extracted intermediate values to make their meaning clearer.
I also moved the if
statements from the match patterns to the match arms so what they do is more explicit and make it more visible that no case is missing.
3c774f5
to
33bbb97
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.
Thank you, that’s much better! One final comment: AFAIS the locations only apply to the keys and PINs are always deleted regardless of the location. So I think renaming locations
to key_locations
would be even clearer.
33bbb97
to
7449dd4
Compare
Factory-reset-app did not fully delete the PINs of the application.
This PR fixes that.