How could this be more idiomatic for Rust?


#1

Hi

I’ve been building a very little image cli, I’m just wondering how it could be better in terms of idiomatic rust? The whole thing is here. But the relevant code is in lib.rs. I had two further questions for clarification:

Firstly, In my function i’m using _ to ignore the result. Am I using it correctly?

pub fn run(config: Config) -> Result<(), Box<Error>> {
   let _ = process(&config.image_path, &config.action); // is this idiomatic rust to ignore the output?
   Ok(())
}

Secondly, and it kind of ties in with the whole “Am I doing this in a rust way/how could it be better” In my process() function, is this the best way to achieve the correct image processing.

pub fn process(file: &String, action: &ActionKind) -> Result<(), Box<Error>> {

    let ref mut img = image::open(file)?;
    let save_location = Path::new("./images").join(file);

    Ok(match action {
        &ActionKind::Gray => imageops::grayscale(img).save(&save_location)?,
        &ActionKind::Crop => imageops::crop(img, 0, 0, 250, 250).to_image().save(&save_location)?,
        &ActionKind::Rotate => println!("Not implemented"),
        &ActionKind::Thumb => println!("Not implemented"),
    })
}

I read @Michael-F-Bryan response to a question a few days ago that said

A common trap you fall into when starting out is to use pattern matching everywhere, leading to excessive rightward drift (populate_files() is nested 6 or 7 levels down) and making code harder to understand. A lot of the time there’s a nice combinator (like Option::and_then()3 or Result::unwrap_or_default()) available which you will be much more expressive and readable than explicit pattern matching.

I feel I definitely at risk of over using the match pattern. Also, in my example, I’ve managed to chain the calls together, I wondered how this process might fair if the function needed on one of the arms had a lot more code?

:slight_smile: :christmas_tree:


#2

Do you really want to ignore the error in run? I’d have expected just something like

pub fn run(config: Config) -> Result<(), Box<Error>> {
   process(&config.image_path, &config.action)
}

In process, consider returning the image from the match, and putting the save after it. I might have the types wrong here, but something like this:

pub fn process(file: &String, action: &ActionKind) -> Result<(), Box<Error>> {

    let ref mut img = image::open(file)?;
    let save_location = Path::new("./images").join(file);

    let new_image = match action {
        &ActionKind::Gray => imageops::grayscale(img),
        &ActionKind::Crop => imageops::crop(img, 0, 0, 250, 250).to_image(),
        &ActionKind::Rotate => unimplemented!(),
        &ActionKind::Thumb => unimplemented!(),
    };

    new_image.save(&save_location)?;
    Ok(())
}

Note also the unimplemented!() macro for the branches that aren’t done yet. It diverges, so I don’t need to come up with an image to use. You could also return an Err, if you prefer.


#3
let _ = process(&config.image_path, &config.action);

This is the idiomatic way to ignore an error in Rust, but… I do not see why you would do that here unless this is just temporary. Especially in a function that returns Result<(), Box<Error>>; it rather feels like that signature is lying to the user, by claiming to propagate errors upwards when in actuality it does not.

If there is a legitimate reason to ignore it, I’d add a comment describing why.

If it is just temporary, I would get rid of the let _ = so that it generates an “unused must-use” warning to remind you to come back and fix it.


println!("Not implemented"),

There is an unimplemented!() macro basically made for this purpose. (It also panics rather than barreling forward).


I’m not sue why you have written action_type as an inner function. If this was a deliberate design choice to help with refactorability or making code self-documenting, that’s alright; but if it was to work around some ergonomic issue, I’ll share how I’d write that:

let action = match env_action {
    "gray" => ActionKind::Gray,
    "thumb" => ActionKind::Thumb,
    "rotate" => ActionKind::Rotate,
    "crop" => ActionKind::Crop,
    s => return Err(format!("Unknown action: {}", s).into()),
    // that last one could also be written as:
    // s => Err(format!("Unknown action: {}", s))?,
};

I feel I definitely at risk of over using the match pattern.

On the contrary! It looks to me like you are already doing a good job of exploring the tools at your disposal.

The main issue with overusing match is when you’re working with types like Option or Result; types that often are used to help with control flow. I don’t see any matches on these types in your code other than an if let Err(e) (which frankly is the least problematic type of match since it’s rare that you’ll ever need complicated nesting inside an Err branch).


#4

@ExpHP Thanks for your help, I forgot to mention why i used if let _ = process(). And that was simply because I was getting the unused must-use. I couldn’t work out how to silence this warning and by silence I meant handling it correctly and I came across this “solution”. I wasn’t sure of how to rewrite my program to stop this?

regarding the inner function, there was no real reason. I just started writing and never really went back and tidied up. I agree though, yours is nicer and clearer :slight_smile:


#5

Most common way to deal with it is to use the ? operator to propagate it back up, as you have done in process.

pub fn run(config: Config) -> Result<(), Box<Error>> {
    process(&config.image_path, &config.action)?;
    Ok(())
}

This is also equivalent to what @scottmcm wrote. Either of these function bodies are fine, and some people will say you should always write it as @scottmcm wrote, but in my personal opinion the two bodies say different things:

This implementation says to the reader, "run() delegates its work to process()." Or, alternatively, it says "run() is just a wrapper around process()":

{
    process(&config.image_path, &config.action)
}

This implementation says to the reader, "run() calls or uses process(). It may also call other functions in the future." It suggests that run() and process() only incidentally have the same output type, and that this may change in the future.

{
    process(&config.image_path, &config.action)?;
    Ok(())
}

#6
    Ok(match action {
        &ActionKind::Gray => imageops::grayscale(img).save(&save_location)?,
        &ActionKind::Crop => imageops::crop(img, 0, 0, 250, 250).to_image().save(&save_location)?,
        &ActionKind::Rotate => println!("Not implemented"),
        &ActionKind::Thumb => println!("Not implemented"),
    })

Instead of matching on a borrow (i.e. the &ActionKind::Gray), it’s nicer to do match *action { ActionKind::Gray => ... }. I think clippy has a lint for that exact use case.

let action = match env_action.as_ref() {
            "gray" => ActionKind::Gray,
            "thumb" => ActionKind::Thumb,
            "rotate" => ActionKind::Rotate,
            "crop" => ActionKind::Crop,
            e @ _ => return Err(format!("Unknown action: {}", e)),
        };

Instead of writing this explicit match to turn a string into an ActionKind which may return an error, you may want to implement FromStr. This lets you do some_string.parse()? and is a lot better at expressing intent.


#7

Thanks @Michael-F-Bryan, never heard of Clippy before, so good to know about that. Be interested to know if it could be hooked into intelliJ which I really like.

I took your advice on the code and dereferenced for the match and implemented the FromStr. That was a nice way to do it! Nice to get a few different styles of how people would/are writing things from this question and a couple other I’ve had! :slight_smile:


#8

It looks like that’s on their radar, but not implemented yet. Either way clippy is super easy to work with. You just install it (cargo +nightly install clippy), then you can run it from the command line like all the other cargo tools and sub-commands.

The +nightly bit is just because clippy uses the compiler and its internals as a library, which are all unstable APIs.