Install multiple binaries from an archive asset #236
Conversation
…r input validation
…hen multiple install targets are specified
|
extract |
There was a problem hiding this comment.
Hi @duong-dt and thanks for your contribution 😄!
I left some comments to check out :)
| executable_names | ||
| .iter() | ||
| .map(|e| Executable::Selected(e.clone())) | ||
| .collect(), |
There was a problem hiding this comment.
- Take ownership of
executable_namesinstead of cloning strings:
executable_names
.into_iter()
.map(Executable::Selected)
.collect(),- Remove duplicates from executable_names to avoid a scenario like this:
$ dra download -I foo -I foo -I foo any/repo
Extracted archive executable to '/home/user/foo'
Extracted archive executable to '/home/user/foo'
Extracted archive executable to '/home/user/foo'
Installation completed!| let destination = self.selection_destination_for_install()?; | ||
| self.destination_may_not_dir(&destination)?; |
There was a problem hiding this comment.
This should be done inside maybe_install function like it was before. This is a specific behaviour of the install feature.
| if self.install.is_more_than_one() { | ||
| match destination { | ||
| Destination::File(x) => Err(HandlerError::new(format!( | ||
| "Multiple install target (-I,--install-file) are selected \ | ||
| but output (-o,--output) is not a directory\n \ | ||
| Output: {:?}", | ||
| x | ||
| ))), | ||
| Destination::Directory(_) => Ok(()), | ||
| } | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Invert the if condition and remove the else branch to have less indentation and be more clear
| let github = GithubClient::from_environment(); | ||
| let release = self.fetch_release(&github)?; | ||
| let selected_asset = self.select_asset(release)?; | ||
| self.asset_may_not_be_archive(&selected_asset.name)?; |
There was a problem hiding this comment.
Not needed here. This is already done when you call installer::install.
Remember to delete the asset_may_not_be_archive function.
| } | ||
|
|
||
| pub fn finish_with_message(&self, message: &str) { | ||
| self.pb.finish_and_clear(); | ||
| println!("{}", message); | ||
| } | ||
|
|
||
| pub fn println(&self, message: &str) { |
| #[cfg(target_family = "unix")] | ||
| #[test_case("helloworld-unix", "helloworld", "helloworld-v2"; "executable")] | ||
| #[test_case("helloworld.deb", "helloworld", "helloworld-v2"; "deb")] | ||
| fn selected_asset_is_not_archive(selected_asset: &str, exec1: &str, exec2: &str) { |
There was a problem hiding this comment.
Not needed since that check is removed. See comments in download_handler.rs
| use assert_cmd::assert::OutputAssertExt; | ||
| use assert_cmd::prelude::CommandCargoExt; | ||
|
|
||
| use crate::assertions::assert_file_exists; | ||
|
|
||
| #[test] | ||
| fn destination_is_not_dir() { |
There was a problem hiding this comment.
Remove this test or transform it in a unit test in download_handler.rs.
End to end tests are expensive to write and maintain. In this case, this one doesn't add much value.
This could be a unit test on download_handler.rs on the destination_may_not_dir function.
|
|
||
| #[cfg(target_family = "windows")] | ||
| #[test_case("helloworld-many-executables-windows.zip", "random-script.exe", "helloworld-v2.exe"; "install 2 executables")] | ||
| fn installed_multiple_successfully(selected_asset: &str, exec1: &str, exec2: &str) { |
There was a problem hiding this comment.
Could you move this test below the one for unix family?
|
|
||
| #[cfg(target_family = "windows")] | ||
| #[test_case("helloworld.exe", "helloworld.exe", "helloworld-v2.exe"; "executable")] | ||
| fn selected_asset_is_not_archive(selected_asset: &str, exec1: &str, exec2: &str) { |
There was a problem hiding this comment.
Not needed since that check is removed. See comments in download_handler.rs
| return Err(HandlerError::new(message)); | ||
| } | ||
|
|
||
| let message = format!("{}\n", Color::new("Installation completed!").green()); |
There was a problem hiding this comment.
Remove \n since it's not needed anymore
This is to support installing multiple binaries from an archive asset #234 , without having to download such asset multiple times, by accepting multiple
-I,--install-filearguments.-I,--install-fileare specified, and-o,--outputis specified, then-o,--outputmust be a directory.-I,--install-fileare specified, the selected asset (either specified by-s,--selector via by-a,--automatic) must be an archive asset.