Repository: starship/starship
File: src/modules/fossil_metrics.rs (Rust)

Code before:

    module.set_segments(match parsed {
        Ok(segments) => segments,
        Err(error) => {
            log::warn!("Error in module `fossil_metrics`:\n{}", error);
            return None;
        }
    });

    Some(module)
}

/// Represents the parsed output from a Fossil diff with the --numstat option enabled.
#[derive(Debug, PartialEq)]
struct FossilDiff<'a> {
    added: &'a str,
    deleted: &'a str,
}

impl<'a> FossilDiff<'a> {
    fn try_parse(diff_numstat: &'a str, only_nonzero_diffs: bool) -> Option<Self> {
        let mut split = match diff_numstat.lines().last() {
            Some(line) => line.split_whitespace(),
            None => {
                log::warn!("Error in module `fossil_metrics`:\nLast line of numstat diff missing.");
                return None;
            }
        };

        let added = match split.next() {
            Some("0") if only_nonzero_diffs => "",
            Some(s) => s,
            None => {
                log::warn!("Error in module `fossil_metrics`:\nNumber of added lines missing.");
                return None;
            }
        };

        let deleted = match split.next() {
            Some("0") if only_nonzero_diffs => "",
            Some(s) => s,
            None => {
                log::warn!("Error in module `fossil_metrics`:\nNumber of deleted lines missing.");
                return None;
            }
        };

        Some(Self { added, deleted })
    }
}


Review comment (refactor):
``Error in module `fossil_metrics`:\n`` is a bit redundant, and the duplicate log message detector may have trouble with this. I would prefer having `try_parse` return a `Result<Self, &str>` instead. Please also add tests for some of the different error conditions.

Code after:
                _ => None,
            })
            .parse(None, Some(context))
    });

    module.set_segments(match parsed {
        Ok(segments) => segments,
        Err(error) => {
            log::warn!("Error in module `fossil_metrics`:\n{}", error);
            return None;
        }
    });

    Some(module)
}

/// Represents the parsed output from a Fossil diff with the --numstat option enabled.
#[derive(Debug, PartialEq)]
struct FossilDiff<'a> {
    added: &'a str,
    deleted: &'a str,
}

impl<'a> FossilDiff<'a> {
    /// Parses the output of `fossil diff --numstat` as a `FossilDiff` struct.
    pub fn parse(diff_numstat: &'a str, only_nonzero_diffs: bool) -> Self {
        // Fossil formats the last line of the output as "%10d %10d TOTAL over %d changed files\n"
        // where the 1st and 2nd placeholders are the number of added and deleted lines respectively
        let re = Regex::new(r"^\s*(\d+)\s+(\d+) TOTAL over \d+ changed files$").unwrap();

        let (added, deleted) = diff_numstat
            .lines()
            .last()
            .and_then(|s| re.captures(s))
            .and_then(|caps| {
                let added = match caps.get(1)?.as_str() {
                    "0" if only_nonzero_diffs => "",
                    s => s,
                };

                let deleted = match caps.get(2)?.as_str() {
                    "0" if only_nonzero_diffs => "",
                    s => s,
                };

                Some((added, deleted))
            })
            .unwrap_or_default();

        Self { added, deleted }
    }