chshersh / tool-sync

🧰 Download pre-built binaries of all your favourite tools with a single command
https://crates.io/crates/tool-sync
Mozilla Public License 2.0
69 stars 16 forks source link

[#110] Introduces the OS enum and improves one error message #124

Closed crudiedo closed 1 year ago

crudiedo commented 1 year ago

Resolves #110

This PR adds the OS enum and updates AssetName logic to use it instead of env::consts::OS.

It also improves a single error message to display Specify 'asset_name.{get_current_os()}' in it.

crudiedo commented 1 year ago

One specific change I wanted to mention\discuss is this part in toml.rs decode_asset_name:

  let linux = str_by_key(table, OS::Linux.to_string().as_str());
  let macos = str_by_key(table, OS::MacOS.to_string().as_str());
  let windows = str_by_key(table, OS::Windows.to_string().as_str());

  AssetName {
      linux,
      macos,
      windows,
  }

It wasn't task of the initial issue, but I believe it makes the design a little bit better by directly showing that we're referring to OS-related value and avoiding the situation when we change the OS keys there, but forgot about that error message

    write!(
        f,
        "Unknown asset selector for the current OS. Specify 'asset_name.{}' in the config.",
        get_current_os()
    )

And it'll be pointing to the wrong config key.