[#54] Adding extra path and added a new error for the case where multiple assets are found

[MitchellBerend]

Resolves #54

This pr add an extra path that gets checked once the binary is downloaded from github. Additionally it also adds an error case where multiple matches are found.

Additional tasks

• Documentation for changes provided/changed
• Tests added

[MitchellBerend]

The config I tested with and its result.

$ cat tools.toml
# This config file was generated for version 0.1.0
#
# # tool-sync default configuration file
# https://github.com/chshersh/tool-sync
# This file was automatically generated by tool-sync
#####################################################
#
#
store_directory = "$HOME/rust/tool-sync/bin"
#
# tool-sync provides native support for some of the tools without the need to configure them
# Uncomment the tools you want to have them
#
# [bat]
# [difftastic]
# [exa]
# [fd]
[ripgrep]
# [tool-sync]
#
# To add configuration for other tools these are the config options:
[github]
        owner     = "cli" 
        repo      = "cli" 
        exe_name  = "gh"

        # Uncomment to download a specific version or tag.
        # Without this tag latest will be used
        tag       = "v2.14.7"

# Asset name to download on linux OSes
        asset_name.linux = "linux_amd64"
#
# uncomment if you want to install on macOS as well
# asset_name.macos = "apple-darwin"
#
# uncomment if you want to install on Windows as well
# asset_name.windows = "x86_64-pc-windows-msvc"

$ cargo run -- --config tools.toml sync 
   Compiling tool-sync v0.1.0 (/home/mitchell/rust/tool-sync)
    Finished dev [unoptimized + debuginfo] target(s) in 2.40s
     Running `target/debug/tool --config tools.toml sync`
❌  github 
Multiple name matches found for this asset:

	 * gh_2.14.7_linux_amd64.deb
	 * gh_2.14.7_linux_amd64.rpm
	 * gh_2.14.7_linux_amd64.tar.gz

Please add one of these to the config.
🔄  All done!                                                                                                                                                             📦  Estimated total download size: 2.01 MiB
✅  ripgrep 13.0.0   Completed!                                                                                                                                           ✨  Successfully installed 1 tools!
📁  Installation directory: /home/mitchell/rust/tool-sync/bin

[chshersh]

This is a helpful message improvement 👍🏻

As always, a few comments 🙂

[chshersh]

I wonder, could we use into_iter() instead of iter() to avoid cloning values and return owned values from the list?

[MitchellBerend]

This does not work here unfortunately since assets is used again on line 90.

[chshersh]

Isn't this an error?

On line 90, you extract name from all original assets fetched from GitHub. I think, it would be better to show only those assets that match the provided query.

So, I imagine, if you add an asset with non-matching name in the multiple_asset_found test, the test would expect this non-matching asset as well. And I think we want to show only matching assets.

Or do I read the code incorrectly? 🤔

[MitchellBerend]

@chshersh The hardcoded gh binary on mac now downloads the amd64 version, it does not support arm (yes) but this might be possible in the future. The same is true for x86 and arm for linux, should there be an error in the realm of:

hey your arch is x but this config/hardcoded tool currently downloads y, add to your .tool.toml

[chshersh]

One more question in the review.

About arm64 — agree, let's keep it for future improvements. Currently, tool-sync matches only on env::consts::OS but when we want to support an arch, we need to handle env::consts::ARCH as well. This will complicate the logic significantly (especially when we want to support automatic asset name guess) so this would require a proper thinking ahead 🙂

[chshersh]

Isn't this an error?

On line 90, you extract name from all original assets fetched from GitHub. I think, it would be better to show only those assets that match the provided query.

So, I imagine, if you add an asset with non-matching name in the multiple_asset_found test, the test would expect this non-matching asset as well. And I think we want to show only matching assets.

Or do I read the code incorrectly? 🤔

[MitchellBerend]

On line 90, you extract name from all original assets fetched from GitHub. I think, it would be better to show only those assets that match the provided query.

The variable naming is not optimal here.

This part has asset contain a Vec<Asset> with only matches.

tool-sync/src/model/tool.rs

Lines 79 to 83 in 265e1f3

	let mut asset = assets
	.iter()
	.filter(|&asset| asset.name.contains(asset_name))
	.map(|asset| asset.to_owned())
	.collect::<Vec<Asset>>();

This part then pulls out the names of all matches.

tool-sync/src/model/tool.rs

Lines 90 to 91 in 265e1f3

	let assets: Vec<String> =
	asset.iter().map(|item| item.name.clone()).collect();

This should probably be ensured in the test though so I will add an asset that does not match.

[chshersh]

Looks terrific 🏆