Yet another Weather CLI

Hello everyone!
As my first Rust project, I've implemented a simple Weather CLI. I really would appreciate your feedback!

The thing that I noticed was this method:

    fn get_weather(&self, api_key: &str, location: &data::Location) -> Result<data::Weather> {
        let response = data_weather(
            api_key,
            location.lat.expect("lat should be set"),
            location.lon.expect("lon should be set"),
        )?;
        let weather: Weather = response.json()?;
        weather.try_into()
    }

It made me curious what the expectation is for checking validity of Locations.

Is there a way you could have a type for a Location that actually has what the function needs, so that it can't fail (AKA Parse, don’t validate or Aiming for correctness with types )? Would it make sense for it to return an Err rather than to panic?

1 Like

@scottmcm Thanks for your feedback!

I also was thinking do there is a way to get rid of those expects. I have them in the first place because the app was designed to have three different weather API providers, but I decided to unify them by using the Provider trait and shared Location and Weather types. However, returned data is different, as some providers return id instead of lat and lon. So, I set those fields as None if they are missing for a specific provider. In such a way, I can use one common Provider trait with three different implementations using the same Location and Weather types.

Also, I found that it seems common in Rust to use panic (e.g., unwrap, expect) in program logic errors, such as broken invariant (reading Why Rust should only have provided `expect` for turning errors into panics, and not also provided `unwrap` :: The Coded Message and Using unwrap() in Rust is Okay - Andrew Gallant's Blog). So, I decided that it's better to handle them as panic, not as Err. Currently, I'm using Err mostly for IO and user-related logic and using panic for coded program logic.

I'm unsure whether this is appropriate and the best way to deal with such a flow, and I would appreciate any feedback if would be.
Thank you!

If different Providers specify locations in different ways, I think the best thing to do would be to make Location an associated type of Provider so that each implementation operates on a location type specific to that provider.

In brief:

pub trait Provider {
    type Location;

    ...

    fn search_location(&self, api_key: &str, q: &str) -> Result<Vec<Self::Location>>;

    fn get_weather(&self, api_key: &str, location: &Self::Location) -> Result<Weather>;
}

impl Provider for SomeProviderThatUsesLatAndLong {
    type Location = LatAndLongLocation;

    ...
}

impl Provider for SomeProviderThatUsesLocationIDs {
    type Location = LocationId;

    ...
}

@jwodder Thanks for your feedback!
I understand how the Provider trait can be changed to use the Location associated type, and in such a way, there would not be a need to use expects. But currently, I also use Location in other places, such as Storage, and save Location in the local config. So, I would need to store somehow different LocationId and LatAndLongLocation, but I'm not sure whether this can be done somehow simply.

That is a good point. The best idea I can come up with (and this is just a suggestion; I'm not saying you should or must do this) would be store locations in the config file using the same structure you have now (ID, lat, long, etc.) but add a StorableLocation trait that LocationId and LatAndLongLocation implement with methods like:

/// Errors if the stored location lacks a field needed by the associated API
fn from_storage(loc: storage::Location) -> Result<Self, Error>;

fn to_storage(self) -> storage::Location;

Then put a StorableLocation bound on the Api (née Provider) trait's associated Location type and use the new conversion methods around the points where you pass locations to/from Api instances. The main benefit of doing this is that, if a malicious or incompetent user modifies the config file to omit crucial fields from a stored location, your program will exit with a helpful error message instead of panicking.

It seems feasible and a great idea to add a new StorableLocation abstraction to handle all possible different Location concrete types. I think it's definitely the way to go in the future if the project gets bigger, but for now, I will leave it as it is for simplicity.
I appreciate your feedback and review. Thank you!

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.