feat: uninstall#95
Conversation
|
@addrian-77 Should we close this? After we have |
addrian-77
force-pushed
the
add/uninstall
branch
2 times, most recently
from
1bc82cf to
91c0c94
Compare
| match installed_apps | ||
| .iter() | ||
| .find(|iter| iter.tbf_header.get_package_name().unwrap_or("") == app_name) | ||
| { | ||
| Some(_) => conn | ||
| .uninstall_app(Some(app_name.to_string()), None) | ||
| .await | ||
| .context("Failed to uninstall app.")?, | ||
| None => panic!("Specified app is not installed!"), |
There was a problem hiding this comment.
Instead of match and panic on none, you can use expect
| let app_name = sub_matches.get_one::<String>("name").map(String::as_str); | ||
|
|
||
| if installed_apps.is_empty() { | ||
| panic!("No apps installed"); |
There was a problem hiding this comment.
I wouldn't panic, I would just write a message to the user.
| #[derive(Debug)] | ||
| pub struct AppOption<'a> { | ||
| pub index: usize, | ||
| pub app: &'a AppAttributes, | ||
| } | ||
|
|
||
| impl<'a> std::fmt::Display for AppOption<'a> { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| if self.index == 0 { | ||
| write!(f, "Delete all") | ||
| } else { | ||
| write!( | ||
| f, | ||
| "{}. {} - start: {:#x}, size: {}, type: {}", | ||
| self.index, | ||
| self.app.tbf_header.get_package_name().unwrap_or(""), | ||
| self.app.address, | ||
| self.app.tbf_header.total_size(), | ||
| if self.app.tbf_header.get_fixed_address_flash().is_none() { | ||
| "C (flexible)" | ||
| } else { | ||
| "Rust (fixed)" | ||
| } | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this structure only used in CLI? What is its purpose?
There was a problem hiding this comment.
Yes, it is used for displaying the list of installed apps.
|
|
||
| let mut address = settings.start_address; | ||
| for app in app_attributes_list.iter() { | ||
| let address = app.address; |
There was a problem hiding this comment.
What is the reasoning behind this change?
There was a problem hiding this comment.
It is simpler and safer this way, the apps can be spaced out in memory and incrementing the address might not work every time. We already have every app's address and size.
| pub trait CommandUninstall { | ||
| async fn uninstall_app( | ||
| &mut self, | ||
| app_name: Option<String>, |
There was a problem hiding this comment.
What does uninstalling an with None name mean?
There was a problem hiding this comment.
When calling the uninstall option we can specify the name of the app (or not), that's why the name is optional. If name is specified, index is None.
| async fn uninstall_app( | ||
| &mut self, | ||
| app_name: Option<String>, | ||
| app_index: Option<usize>, |
There was a problem hiding this comment.
What is this index for?
There was a problem hiding this comment.
This parameter is the selected index from the provided apps list. When --name is not used, app_name is None and index is Some(value). There is no scenario in which both have Some() or None values.
eva-cosma
left a comment
eva-cosma
left a comment
There was a problem hiding this comment.
I am a bit confused why you made some of the design choices like you did. I would like some doc comments that explain it for both the user and me why what and how
Signed-off-by: Adrian Lungu <lunguadrian30@gmail.com>
addrian-77
force-pushed
the
add/uninstall
branch
from
91c0c94 to
eacf53d
Compare
2 participants
Pull Request Overview
This pull request adds the uninstall feature
TODO or Help Wanted
Checks
Using Rust tooling
cargo fmtcargo clippycargo testcargo buildFeatures tested:
GitHub Issue
This pull request closes <Implement uninstall #88 >