Skip to content
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

[#83] Create 'AssetError' and refactor 'select_asset' function to return it #97

Merged
merged 11 commits into from
Sep 18, 2022
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ store_directory = "~/.local/bin"
By default `tool-sync` reads configuration from `$HOME/.tool.toml` you can run `tool
default-config` to print a default configuration example to std out. You can
redirect this out put to a file like so `tool default-config >
$HOME/.tools.toml`.
$HOME/.tool.toml`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation fix! 🙏🏻


You can also quickly copy the above configuration to the default path by running
the following command (Unix-only):
Expand Down
40 changes: 39 additions & 1 deletion src/model/release.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,52 @@
use serde::Deserialize;
use std::fmt::{Display, Formatter};

#[derive(Deserialize, Debug)]
pub struct Release {
pub tag_name: String,
pub assets: Vec<Asset>,
}

#[derive(Deserialize, Debug, Clone)]
#[derive(Deserialize, Debug, Clone, Eq, PartialEq)]
pub struct Asset {
pub id: u32,
pub name: String,
pub size: u64,
}

#[derive(Debug, PartialEq, Eq)]
pub enum AssetError {
/// Asset name of this OS is unknown
NameUnknown,

/// Asset name is not in the fetched assets
NotFound(String),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NameUnknown and NotFound sounds too similar in a semantic sense to me. I could switch those two names and enum still would make sense to me (both OS and Asset name can be either unknown or not found).

In this context, NotFound is fine but let's rename NameUnknown to OsSelectorUnknown. Just to give some connection to the fact that it's more related to OS configuration and not the asset name in the list of assets.

Copy link
Contributor Author

@zixuan-x zixuan-x Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I struggled with naming these two errors, OsSelectorUnknown makes sense

}

impl Display for AssetError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
Self::NameUnknown => {
write!(
f,
"Don't know the asset name for this OS: specify it explicitly in the config"
)
zixuan-x marked this conversation as resolved.
Show resolved Hide resolved
}
Self::NotFound(asset_name) => {
write!(f, "No asset matching name: {}", asset_name)
}
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn display_asset_error() {
let asset_name = "test_asset";
let error_str = AssetError::NotFound(asset_name.to_string()).to_string();
assert_ne!(error_str.find(asset_name), None);
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this test. It doesn't test much so there's no need to maintain it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, testing error messages is generally brittle

99 changes: 94 additions & 5 deletions src/model/tool.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::release::Asset;
use crate::infra::client::Client;
use crate::model::asset_name::AssetName;
use crate::model::release::AssetError;

#[derive(Debug, PartialEq, Eq)]
pub enum Tool {
Expand Down Expand Up @@ -69,16 +70,15 @@ pub struct ToolInfo {
}

impl ToolInfo {
pub fn select_asset(&self, assets: &[Asset]) -> Result<Asset, String> {
/// Select an Asset from all Assets based on which Operating System is used
pub fn select_asset(&self, assets: &[Asset]) -> Result<Asset, AssetError> {
match self.asset_name.get_name_by_os() {
None => Err(String::from(
"Don't know the asset name for this OS: specify it explicitly in the config",
)),
None => Err(AssetError::NameUnknown),
Some(asset_name) => {
let asset = assets.iter().find(|&asset| asset.name.contains(asset_name));

match asset {
None => Err(format!("No asset matching name: {}", asset_name)),
None => Err(AssetError::NotFound(asset_name.to_owned())),
Some(asset) => Ok(asset.clone()),
}
}
Expand Down Expand Up @@ -107,3 +107,92 @@ pub struct ToolAsset {
/// GitHub API client that produces the stream for downloading the asset
pub client: Client,
}

#[cfg(test)]
mod tests {
use super::*;
Comment on lines +111 to +113
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on tests! 👏🏻

Let's also add one more test for the OsSelectorUknown constructor 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. added


#[test]
fn asset_fount() {
let asset_name = "asset";

let tool_info = ToolInfo {
owner: "owner".to_string(),
repo: "repo".to_string(),
exe_name: "exe".to_string(),
tag: ToolInfoTag::Latest,
asset_name: AssetName {
linux: Some(asset_name.to_string()),
macos: Some(asset_name.to_string()),
windows: Some(asset_name.to_string()),
},
};

let assets = vec![
Asset {
id: 1,
name: "1".to_string(),
size: 10,
},
Asset {
id: 2,
name: asset_name.to_string(),
size: 50,
},
Asset {
id: 3,
name: "3".to_string(),
size: 77,
},
];

assert_eq!(
tool_info.select_asset(&assets),
Ok(Asset {
id: 2,
name: asset_name.to_string(),
size: 50
})
);
}

#[test]
fn asset_not_found() {
let asset_name = "asset";

let tool_info = ToolInfo {
owner: "owner".to_string(),
repo: "repo".to_string(),
exe_name: "exe".to_string(),
tag: ToolInfoTag::Latest,
asset_name: AssetName {
linux: Some(asset_name.to_string()),
macos: Some(asset_name.to_string()),
windows: Some(asset_name.to_string()),
},
};

let assets = vec![
Asset {
id: 1,
name: "1".to_string(),
size: 10,
},
Asset {
id: 2,
name: "2".to_string(),
size: 50,
},
Asset {
id: 3,
name: "3".to_string(),
size: 77,
},
];

assert_eq!(
tool_info.select_asset(&assets),
Err(AssetError::NotFound(asset_name.to_string()))
);
}
}
4 changes: 2 additions & 2 deletions src/sync/prefetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ fn prefetch_tool(
None
}
Ok(release) => match tool_info.select_asset(&release.assets) {
Err(msg) => {
prefetch_progress.unexpected_err_msg(tool_name, &msg);
Err(err) => {
prefetch_progress.unexpected_err_msg(tool_name, &err.to_string());
prefetch_progress.update_message(already_completed);
None
}
Expand Down