Create 'AssetError' and test 'select_asset' function

[chshersh]

This function selects an asset to download based on config:

https://github.com/chshersh/tool-sync/blob/b79eeb91cfdc3a122e6693d503117f68ff1fb44e/src/model/tool.rs#L72-L86

Currently, it returns Result<Asset, String> but the goal is to return a custom type for error: Result<Asset, AssetError.

The idea is to replace String a custom constructor and add tests for these cases.

• [x] Create a new enum with two constructors
• [x] Implement the std::fmt::Display trait
• [x] Replace String with enum
• [x] Write tests

[MitchellBerend]

Implement a display() function for this enum

Should we not implement std::fmt::Display? This way format!, print!, println! and eprint! for example will all work without having to call display on the error explicitly.

[chshersh]

Hmm, I didn't know about Display 🤔 Is this the standard and common way to format data types in Rust?

I don't mind using a more standard way of implementing things 🙂 I believe I defined the display function manually in several places. So maybe we could start with the refactoring of these functions to use Display instead?

src/config/toml.rs
17:    pub fn display(&self) -> String {

src/model/tool.rs
21:    pub fn display(&self) -> String {

src/sync/archive.rs
28:    pub fn display(&self) -> String {

[MitchellBerend]

This Display trait is like pythons __repr__ if you know that.

Here is some example:

use std::fmt;

enum TestEnum {
    Var1(String),
    Var2(String),
}

impl fmt::Display for TestEnum {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            TestEnum::Var1(s) => write!(f, "[VAR1]: {}", s),
            TestEnum::Var2(s) => write!(f, "[VAR2]: {}", s),
        }
    }
}

fn main() {
    let error1 = TestEnum::Var1("error 1".into());
    let error2 = TestEnum::Var2("error 2".into());

    println!("{}", error1);
    eprintln!("{}", error2);
}

[chshersh]

This looks great! 👏🏻 In that case, let's go with std::fmt::Display instead of our own display function. I've edited the issue description.

I like learning about such things about idiomatic Rust code and what are the standard best practices 😌

[MitchellBerend]

Im not sure how invasive this change will be but I was thinking that all functions that take some error can be made generic over all types that implement Display. That way the error itself can be passed instead of having to convert it manually first.

pub fn abort_with<Message: Display>(err_msg: Message) -> ! {
    eprintln!(
        r#"Aborting 'tool-sync' with error:

    * {}"#,
        err_msg
    );

    process::exit(1);
}

//These should all work

abort_with("Some non descriptive error");

abort_with(TomlError::IO("The machine is OOM".into()));

[chshersh]

Sounds like a good change 🙂 Feel free to open a PR with the required refactoring changes. I don't expect this change to be invasive either so happy to merge it.

[MitchellBerend]

Id like to leave good-first-issues for others so they can get familiar with the code base. So unless this is blocking some other issue I will leave this open.

[chshersh]

@MitchellBerend That's an excellent idea, I completely agree with this 🆗

I've created a separate issue to refactor usages of the existing display() method to use the Display typeclass:

• https://github.com/chshersh/tool-sync/issues/88

Feel free to create an issue for better abort_with functions 👍🏻 I might create them as well by myself later once #88 is done but you can go ahead and create them straightway.

[zixuan-x]

Hello, I'm a Rust beginner and I saw this awesome project! This development-tools as code concept is really cool and I'd love to contribute to it!

If there's no one working on this issue already, may I start working on it?

[MitchellBerend]

I'm not working on this.

[chshersh]

@zixuanzhang-x No one is working so feel free to start 🙂 Really appreciate contributions 🤗