COMBINE-lab / simpleaf

A rust framework to make using alevin-fry even simpler
BSD 3-Clause "New" or "Revised" License
45 stars 3 forks source link

Typo fix, and rust syntax #44

Closed robsyme closed 1 year ago

robsyme commented 1 year ago

Overview

Two small spelling changes, and two small syntax changes to shorten the execute_command function.

Spelling

The spelling changes are 'unsuccesful' to 'unsuccessful'

Rust-isms

In simplified form, we start with:

match cmd.output() {
    Ok(output) => {
        if output.status.success() {
            // Log success to info
            match verbosity_level {
                CommandVerbosityLevel::Verbose => {
                    // Log stderr and stdout to info
                }
                CommandVerbosityLevel::Quiet => {}
            }
            Ok(output)
        } else {
            // Log failure
            // Log stderr and stdout to error
            Ok(output)
        }
    }
    Err(e) => {
        // Log error
    }
}

The first syntax change is to use match guards to inline the if statement:

match cmd.output() {
    Ok(output) if output.status.success() => {
        // Log success to info
        match verbosity_level {
            CommandVerbosityLevel::Verbose => {
                // Log stderr and stdout to info
            }
            CommandVerbosityLevel::Quiet => {}
        }
    }
    Ok(output) => {
        // Log failure
        // Log stderr and stdout to error
    }
    Err(e) => {
        // Log error
    }
}

Lastly, we can use if let to simplify the verbosity check and remove the empty match arm

match cmd.output() {
    Ok(output) if output.status.success() => {
        // Log success to info
        if let CommandVerbosityLevel::Verbose = verbosity_level {
            // Log stderr and stdout to info
        }
    }
    Ok(output) => {
        // Log failure
        // Log stderr and stdout to error
    }
    Err(e) => {
        // Log error
    }
}
rob-p commented 1 year ago

Hi @robsyme,

Thank you for the contribution and for the detailed description! I think these are all great and will merge in this PR. The only change I might roll back is the last one — that is the conversion from the match to the if let CommandVerbosityLevel::Verbose = verbosity_level. The reason is that I anticipate that, in the future, we may want to expand the potential set of variants in CommandVerbosityLevel, and when the Enum is no longer binary, the match construct may make more sense. However, I agree that for binary Enums in general, this is the cleaner solution. The other changes look great (and robust to future expansion).

--Rob