[#83] Create 'AssetError' and refactor 'select_asset' function to return it #97
Conversation
| @@ -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`. | |||
There was a problem hiding this comment.
Thanks for the documentation fix! 🙏🏻
src/model/release.rs
Outdated
| #[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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's remove this test. It doesn't test much so there's no need to maintain it
There was a problem hiding this comment.
agree, testing error messages is generally brittle
src/model/release.rs
Outdated
| /// Asset name of this OS is unknown | ||
| NameUnknown, | ||
|
|
||
| /// Asset name is not in the fetched assets | ||
| NotFound(String), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah I struggled with naming these two errors, OsSelectorUnknown makes sense
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; |
There was a problem hiding this comment.
Great work on tests! 👏🏻
Let's also add one more test for the OsSelectorUknown constructor 👍🏻
Resolves #83
Refactored
ToolInfo.select_asset(&assets)to return a customAssetErrorerror instead of a string and added tests for the function.The
AssetErrorenum has two possible values:NameUnknown: this is whenAssetNamedoes not know what the asset name is for a specific OS the user uses.NotFound: this is when theAssetNameis not in theAssetsslice passed into theselect_asset(&assets)method.Implemented
std::fmt::Displaytrait forAssetErrorand added tests for it.Additional tasks