Code Review Comments Rust

Code Review Comments/Preferences for mSupply Rust Code

Run Clippy

Installing and running clippy can help identify a lot of common rust style issues, see https://github.com/rust-lang/rust-clippy

Unfortunately, a lot of our code doesn't follow clippy style guides right now, so it can be hard to identify where your code represented amongst other errors.

The list of clippy lints can be found here: https://rust-lang.github.io/rust-clippy/master/index.html

Prefer .to_string()

When working with strings in rust, there's a number of ways to convert a str value into a Owned String.

Given this code: let hello = "Hello World";

We prefer using

let hello = hello.to_string();

This is fast to type, is easy to read, and logically the easiest to follow for less experienced rust developers.

Avoid alternatives such as let hello = hello.to_owned(); or let hello = String::from(hello);

Record modified_datetime and created_datetime in database records

When investigating an issue, it is often helpful to see when a record was first created and when it was last modified. For most records that can be updated (e.g. master_data records, user accounts, items, etc) modified_datetime and created_datetime should be recorded in the database.

Record user_id in database records

When investigating an issue, it is often helpful to see who last modified a record.

Don't use unwrap() unless in tests

When you make a call to unwrap() if an error was to occur in the code the whole server could crash. In tests this is ok, but in production code it should almost never be used. If you have a use case where unwrap does make sense (say if you can't connect to the database on server start up) this useage must include a comment and justification. See Also: https://levelup.gitconnected.com/rust-never-use-unwrap-in-production-c123b311f620

Avoid indirection with From implementation

Avoid using From implementation, unless it's for a common type like RepositoryError.

Generally in Rust, From implementation is used to translate between types, then translations are done with .into() or ? (for errors), this created a bit of indirection which is harder to trace (navigation to implementation or definition in IDE brings you to trait implementation). Although it's possible to navigate to actual implementation of the trait (by examining implementations of a type), it's much easier to navigate to a standard method for a type, like to_domain. And when re-mapping errors, it's more clearly done inline.

// Preferred:

let result = validate_parameter(&parameter).map_err(|e| match e {
     ValidateParameterResult::One(extra) => ValidateResult::One(extra),
     ValidateParameterResult::Two => ValidateResult::Two,
     ValidateParameterResult::Three => ValidateResult::Three,
})?;

// vs To be avoided:

let result = validate_parameter(&parameter)?; // Can't navigate to `?`

// ..

impl From<ValidateParameterResult> for ValidateResult {
    fn from(result: ValidateParameterResult) -> ValidateResult {
        match e {
            ValidateParameterResult::One(extra) => ValidateResult::One(extra),
            ValidateParameterResult::Two => ValidateResult::Two,
            ValidateParameterResult::Three => ValidateResult::Three,
        }
    }
}


// Preferred:

let result = get_record().map(RecordNode::to_domain)?;

// ..

impl RecordNode {
    fn do_domain(row: RecordRow) -> RecordNode {
        //  mapper
    }
}

// vs To be avoided:

let result = get_record().map(RecordNode::from)?;

// or

let result = get_record().map(|r| r.into())?;

// ..

impl From<RecordRow> for RecordNode {
    fn from(row: RecordRow) -> RecordNode {
        //  mapper
    }
}