ronantakizawa/codereview-bench huggingface dataset:
Repository: starship/starship
File: tests/testsuite/shlvl.rs (Rust)

Code before:

#[test]
fn custom_symbol() -> io::Result<()> {
    let output = common::render_module("shlvl")
        .env_clear()
        .use_config(toml::toml! {
            [shlvl]
            symbol = "shlvl is "
            disabled = false
        })
        .env(SHLVL_ENV_VAR, "2")
        .output()?;
    let expected = format!("{} ", style().paint("shlvl is 2"));
    let actual = String::from_utf8(output.stdout).unwrap();
    assert_eq!(expected, actual);
    Ok(())
}

#[test]
fn formatting() -> io::Result<()> {
    let output = common::render_module("shlvl")
        .env_clear()
        .use_config(toml::toml! {
            [shlvl]
            format = "$symbol going down [$shlvl]($style) GOING UP "
            suffix = " level(s) "
            disabled = false
        })
        .env(SHLVL_ENV_VAR, "2")
        .output()?;
    let expected = format!("↕️   going down {} GOING UP ", style().paint("2"));
    let actual = String::from_utf8(output.stdout).unwrap();
    assert_eq!(expected, actual);
    Ok(())
}


Review comment (suggestion):
I'm not sure this line does anything should this be removed?

```suggestion
```

Code after:

#[test]
fn custom_symbol() -> io::Result<()> {
    let output = common::render_module("shlvl")
        .env_clear()
        .use_config(toml::toml! {
            [shlvl]
            symbol = "shlvl is "
            disabled = false
        })
        .env(SHLVL_ENV_VAR, "2")
        .output()?;
    let expected = format!("{} ", style().paint("shlvl is 2"));
    let actual = String::from_utf8(output.stdout).unwrap();
    assert_eq!(expected, actual);
    Ok(())
}

#[test]
fn formatting() -> io::Result<()> {
    let output = common::render_module("shlvl")
        .env_clear()
        .use_config(toml::toml! {
            [shlvl]
            format = "$symbol going down [$shlvl]($style) GOING UP "
            disabled = false
        })
        .env(SHLVL_ENV_VAR, "2")
        .output()?;
    let expected = format!("↕️   going down {} GOING UP ", style().paint("2"));
    let actual = String::from_utf8(output.stdout).unwrap();
    assert_eq!(expected, actual);
    Ok(())
}
Repository: starship/starship
File: src/module.rs (Rust)

Code before:
    "swift",
    "terraform",
    "shlvl",
    "singularity",
    "time",
    "username",
    "zig",
];

/// A module is a collection of segments showing data for a single integration
/// (e.g. The git module shows the current git branch and status)
pub struct Module<'a> {
    /// The module's configuration map if available
    pub config: Option<&'a toml::Value>,

    /// The module's name, to be used in configuration and logging.
    name: String,

    /// The module's description
    description: String,

    /// The collection of segments that compose this module.
    pub segments: Vec<Segment>,

    /// the time it took to compute this module
    pub duration: Option<Duration>,
}

impl<'a> Module<'a> {
    /// Creates a module with no segments.
    pub fn new(name: &str, desc: &str, config: Option<&'a toml::Value>) -> Module<'a> {
        Module {
            config,
            name: name.to_string(),
            description: desc.to_string(),
            segments: Vec::new(),
            duration: None,
        }
    }

    /// Set segments in module
    pub fn set_segments(&mut self, segments: Vec<Segment>) {
        self.segments = segments;
    }

    /// Get module's name
    pub fn get_name(&self) -> &String {
        &self.name
    }

    /// Get module's description

Review comment (suggestion):
I think this could be plain `Duration`, initiliazed as `Duration::default()` (0) when it is not set yet.

Code after:
    "swift",
    "terraform",
    "shlvl",
    "singularity",
    "time",
    "username",
    "zig",
];

/// A module is a collection of segments showing data for a single integration
/// (e.g. The git module shows the current git branch and status)
pub struct Module<'a> {
    /// The module's configuration map if available
    pub config: Option<&'a toml::Value>,

    /// The module's name, to be used in configuration and logging.
    name: String,

    /// The module's description
    description: String,

    /// The collection of segments that compose this module.
    pub segments: Vec<Segment>,

    /// the time it took to compute this module
    pub duration: Duration,
}

impl<'a> Module<'a> {
    /// Creates a module with no segments.
    pub fn new(name: &str, desc: &str, config: Option<&'a toml::Value>) -> Module<'a> {
        Module {
            config,
            name: name.to_string(),
            description: desc.to_string(),
            segments: Vec::new(),
            duration: Duration::default(),
        }
    }

    /// Set segments in module
    pub fn set_segments(&mut self, segments: Vec<Segment>) {
        self.segments = segments;
    }

    /// Get module's name
    pub fn get_name(&self) -> &String {
        &self.name
    }

    /// Get module's description
Repository: starship/starship
File: install/install.sh (Shell)

Code before:
  # https://stackoverflow.com/a/11655875
  local good
  good=$(
    IFS=:
    for path in $PATH; do
      if [ "${path}" = "${bin_dir}" ]; then
        echo 1
        break
      fi
    done
  )

  if [ "${good}" != "1" ]; then
    warn "Bin directory ${bin_dir} is not in your \$PATH"
  fi
}

is_build_available() {
  local arch="$1"
  local platform="$2"

  local target="${arch}-${platform}"
  local good
  
  good=$(
    for t in x86_64-unknown-linux-gnu x86_64-unknown-linux-musl i686-unknown-linux-musl x86_64-apple-darwin x86_64-pc-windows-msvc; do
      if [ "${t}" == "${target}" ]; then
        echo 1
        break
      fi
    done
  )

  if [ "${good}" != "1" ]; then
    error "${arch} builds for ${platform} are not yet available for Starship"
    printf "\n" >&2
    info "If you would like to see a build for your configuration,"
    info "please create an issue requesting a build for ${MAGENTA}${target}${NO_COLOR}:"
    info "${BOLD}${UNDERLINE}https://github.com/starship/starship/issues/new/${NO_COLOR}"
    printf "\n"
    exit 1
  fi
}

# defaults
if [ -z "${PLATFORM-}" ]; then
  PLATFORM="$(detect_platform)"
fi

if [ -z "${BIN_DIR-}" ]; then
  BIN_DIR=/usr/local/bin

Review comment (refactor):
Could we move this to some array (or pseudo-array) so that there's a single obvious source of truth for which platforms are supported by the install script?

If that's too much of a pain in POSIX I'm not too attached to this request.

Code after:
  fi

  # https://stackoverflow.com/a/11655875
  local good
  good=$(
    IFS=:
    for path in $PATH; do
      if [ "${path}" = "${bin_dir}" ]; then
        echo 1
        break
      fi
    done
  )

  if [ "${good}" != "1" ]; then
    warn "Bin directory ${bin_dir} is not in your \$PATH"
  fi
}

is_build_available() {
  local arch="$1"
  local platform="$2"

  local target="${arch}-${platform}"
  local good
  
  good=$(
    IFS=" "
    for t in $SUPPORTED_TARGETS; do
      if [ "${t}" == "${target}" ]; then
        echo 1
        break
      fi
    done
  )

  if [ "${good}" != "1" ]; then
    error "${arch} builds for ${platform} are not yet available for Starship"
    printf "\n" >&2
    info "If you would like to see a build for your configuration,"
    info "please create an issue requesting a build for ${MAGENTA}${target}${NO_COLOR}:"
    info "${BOLD}${UNDERLINE}https://github.com/starship/starship/issues/new/${NO_COLOR}"
    printf "\n"
    exit 1
  fi
}

# defaults
if [ -z "${PLATFORM-}" ]; then
  PLATFORM="$(detect_platform)"
fi
Repository: starship/starship
File: src/modules/zig.rs (Rust)

Code before:
use super::{Context, Module, RootModuleConfig};

use crate::configs::zig::ZigConfig;
use crate::formatter::StringFormatter;

/// Creates a module with the current Zig version
///
/// Will display the Zig version if any of the following criteria are met:
///     - The current directory contains a file with extension `.zig`
pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
    let mut module = context.new_module("zig");
    let config = ZigConfig::try_load(module.config);

    let is_zig_project = context
        .try_begin_scan()?
        .set_files(&config.detect_files)
        .set_extensions(&config.detect_extensions)
        .set_folders(&config.detect_folders)
        .is_match();

    if !is_zig_project {
        return None;
    }

    let parsed = StringFormatter::new(config.format).and_then(|formatter| {
        formatter
            .map_meta(|variable, _| match variable {
                "symbol" => Some(config.symbol),
                _ => None,
            })
            .map_style(|variable| match variable {
                "style" => Some(Ok(config.style)),
                _ => None,

Review comment (refactor):
Not a blocker but I think we can remove this comment.
```suggestion
```

Code after:
use super::{Context, Module, RootModuleConfig};

use crate::configs::zig::ZigConfig;
use crate::formatter::StringFormatter;

/// Creates a module with the current Zig version
pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
    let mut module = context.new_module("zig");
    let config = ZigConfig::try_load(module.config);

    let is_zig_project = context
        .try_begin_scan()?
        .set_files(&config.detect_files)
        .set_extensions(&config.detect_extensions)
        .set_folders(&config.detect_folders)
        .is_match();

    if !is_zig_project {
        return None;
    }

    let parsed = StringFormatter::new(config.format).and_then(|formatter| {
        formatter
            .map_meta(|variable, _| match variable {
                "symbol" => Some(config.symbol),
                _ => None,
            })
            .map_style(|variable| match variable {
                "style" => Some(Ok(config.style)),
                _ => None,
            })
            .map(|variable| match variable {
                "version" => {
Repository: starship/starship
File: src/modules/git_metrics.rs (Rust)

Code before:
        diff.lines()
            .filter(|line| re.is_match(line))
            .count()
            .to_string(),
    )
}
fn get_modified_lines(diff: &str) -> Option<String> {
    let re = Regex::new(r"^.+(\[-|\{\+).*$").unwrap();

    Some(
        diff.lines()
            .filter(|line| re.is_match(line))
            .count()
            .to_string(),
    )
}
fn get_deleted_lines(diff: &str) -> Option<String> {
    let re = Regex::new(r"^\[-.*-\]$").unwrap();

    Some(
        diff.lines()
            .filter(|line| re.is_match(line))
            .count()
            .to_string(),
    )
}

#[cfg(test)]
mod tests {
    use std::ffi::OsStr;
    use std::fs::OpenOptions;
    use std::io::{self, Error, ErrorKind, Write};
    use std::path::{Path, PathBuf};
    use std::process::{Command, Stdio};

    use ansi_term::Color;

    use crate::test::ModuleRenderer;

    #[test]
    fn shows_nothing_on_empty_dir() -> io::Result<()> {
        let repo_dir = tempfile::tempdir()?;

        let actual = ModuleRenderer::new("git_metrics")
            .path(repo_dir.path())
            .collect();

        let expected = None;

        assert_eq!(expected, actual);
        repo_dir.close()

Review comment (suggestion):
Instead of iterating through the diff three times, it may be simpler and more declarative to have a `GitDiff` struct that parses output a single time:

```rust
struct GitDiff {
  added: u32,
  modified: u32,
  deleted: u32,
}

impl GitDiff {
  fn parse(diff: &str) -> Self;
}

```

Code after:

/// Represents the parsed output from a git diff.
struct GitDiff<'a> {
    added: &'a str,
    deleted: &'a str,
}

impl<'a> GitDiff<'a> {
    /// Returns the first capture group given a regular expression and a string.
    /// If it fails to get the capture group it will return "0".
    fn get_matched_str(diff: &'a str, re: &Regex) -> &'a str {
        match re.captures(diff) {
            Some(caps) => caps.get(1).unwrap().as_str(),
            _ => "0",
        }
    }

    /// Parses the result of 'git diff --shortstat' as a `GitDiff` struct.
    pub fn parse(diff: &'a str) -> Self {
        let added_re = Regex::new(r"(\d+) \w+\(\+\)").unwrap();
        let deleted_re = Regex::new(r"(\d+) \w+\(\-\)").unwrap();

        Self {
            added: GitDiff::get_matched_str(diff, &added_re),
            deleted: GitDiff::get_matched_str(diff, &deleted_re),
        }
    }
}

#[cfg(test)]
mod tests {
    use std::ffi::OsStr;
    use std::fs::OpenOptions;
    use std::io::{self, Error, ErrorKind, Write};
    use std::path::{Path, PathBuf};
    use std::process::{Command, Stdio};

    use ansi_term::Color;

    use crate::test::ModuleRenderer;

    #[test]
    fn shows_nothing_on_empty_dir() -> io::Result<()> {
        let repo_dir = tempfile::tempdir()?;
        let path = repo_dir.path();

        let actual = render_metrics(path);

        let expected = None;

        assert_eq!(expected, actual);
Repository: starship/starship
File: src/modules/git_metrics.rs (Rust)

Code before:
use regex::Regex;

use crate::{
    config::RootModuleConfig, configs::git_metrics::GitMetricsConfig, formatter::StringFormatter,
    module::Module,
};

use super::Context;

/// Creates a module with the current added/modified/deleted lines in the git repository at the
/// current directory
pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
    let mut module = context.new_module("git_metrics");
    let config: GitMetricsConfig = GitMetricsConfig::try_load(module.config);

    // As we default to disabled=true, we have to check here after loading our config module,
    // before it was only checking against whatever is in the config starship.toml
    if config.disabled {
        return None;
    };

    let repo = context.get_repo().ok()?;
    let repo_root = repo.root.as_ref()?;

    let diff = context
        .exec_cmd(
            "git",
            &[
                "-C",
                &repo_root.to_string_lossy(),
                "--no-optional-locks",
                "diff",
                "--shortstat",
            ],
        )?

Review comment (suggestion):
```suggestion
/// Creates a module with the current added/deleted lines in the git repository at the
```

Code after:
use regex::Regex;

use crate::{
    config::RootModuleConfig, configs::git_metrics::GitMetricsConfig, formatter::StringFormatter,
    module::Module,
};

use super::Context;

/// Creates a module with the current added/deleted lines in the git repository at the
/// current directory
pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
    let mut module = context.new_module("git_metrics");
    let config: GitMetricsConfig = GitMetricsConfig::try_load(module.config);

    // As we default to disabled=true, we have to check here after loading our config module,
    // before it was only checking against whatever is in the config starship.toml
    if config.disabled {
        return None;
    };

    let repo = context.get_repo().ok()?;
    let repo_root = repo.root.as_ref()?;

    let diff = context
        .exec_cmd(
            "git",
            &[
                "-C",
                &repo_root.to_string_lossy(),
                "--no-optional-locks",
                "diff",
                "--shortstat",
            ],
        )?
Repository: starship/starship
File: src/modules/sudo.rs (Rust)

Code before:
    });

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

    Some(module)
}

#[cfg(test)]
mod tests {
    use crate::{test::ModuleRenderer, utils::CommandOutput};

    #[test]
    fn test_sudo_not_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd(
                "command -v sudo",
                Some(CommandOutput {
                    stdout: "/usr/bin/sudo".to_string(),
                    stderr: "".to_string(),
                }),
            )
            .config(toml::toml! {
                [sudo]
                disabled = false
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }
}


Review comment (bug):
I think the tests are failing because you are mocking the wrong command here. I think this should be

```suggestion
            .cmd(
                "sudo -n true",
                None),
```


Code after:
            })
            .map_style(|variable| match variable {
                "style" => Some(Ok(config.style)),
                _ => None,
            })
            .parse(None, Some(context))
    });

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

    Some(module)
}

#[cfg(test)]
mod tests {
    use crate::{test::ModuleRenderer, utils::CommandOutput};
    use ansi_term::Color;

    #[test]
    fn test_sudo_not_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("sudo -n true", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
                allow_windows = true
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }

    #[test]
    fn test_sudo_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd(
                "sudo -n true",
                Some(CommandOutput {
                    stdout: "".to_owned(),
                    stderr: "".to_owned(),
                }),
            )
            .config(toml::toml! {
                [sudo]
Repository: starship/starship
File: src/modules/sudo.rs (Rust)

Code before:
        let expected = Some(format!("{}", Color::Blue.bold().paint("as 🧙‍ ")));

        assert_eq!(expected, actual);
    }

    #[test]
    #[cfg(not(windows))]
    fn test_doas_not_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("doas -n true", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
                binary = "doas"
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }

    #[test]
    #[cfg(not(windows))]
    fn test_doas_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("doas true", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
                binary = "doas"
            })
            .collect();
        let expected = Some(format!("{}", Color::Blue.bold().paint("as 🧙‍ ")));

        assert_eq!(expected, actual);
    }

    #[test]
    #[cfg(windows)]
    fn test_allow_windows_disabled_blocks_windows() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("sudo -v", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
                allow_windows = false
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);

Review comment (suggestion):
I think this is missing the `-n` flag:
```suggestion
            .cmd("doas -n true", None)
```

Code after:
                [sudo]
                disabled = false
                allow_windows = true
            })
            .collect();
        let expected = Some(format!("{}", Color::Blue.bold().paint("as 🧙 ")));

        assert_eq!(expected, actual);
    }

    #[test]
    #[cfg(windows)]
    fn test_allow_windows_disabled_blocks_windows() {
        let actual = ModuleRenderer::new("sudo")
            .cmd(
                "sudo -n true",
                Some(CommandOutput {
                    stdout: "".to_owned(),
                    stderr: "".to_owned(),
                }),
            )
            .config(toml::toml! {
                [sudo]
                disabled = false
                allow_windows = false
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }
}
Repository: starship/starship
File: src/modules/sudo.rs (Rust)

Code before:
                "style" => Some(Ok(config.style)),
                _ => None,
            })
            .parse(None)
    });

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

    Some(module)
}

#[cfg(test)]
mod tests {
    use crate::test::ModuleRenderer;
    use ansi_term::Color;

    #[test]
    #[cfg(not(windows))]
    fn test_sudo_not_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("sudo -k", None)
            .cmd("sudo -n true", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }

    #[test]
    #[cfg(not(windows))]
    fn test_sudo_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("sudo -v", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
            })
            .collect();
        let expected = Some(format!("{}", Color::Blue.bold().paint("as 🧙 ")));

        assert_eq!(expected, actual);

Review comment (suggestion):
Please mock `sudo -n true` for every test. Please don't need to mock other commands like `sudo -k` or `sudo -v` that will not be executed.

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

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

    Some(module)
}

#[cfg(test)]
mod tests {
    use crate::{test::ModuleRenderer, utils::CommandOutput};
    use ansi_term::Color;

    #[test]
    fn test_sudo_not_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd("sudo -n true", None)
            .config(toml::toml! {
                [sudo]
                disabled = false
                allow_windows = true
            })
            .collect();
        let expected = None;

        assert_eq!(expected, actual);
    }

    #[test]
    fn test_sudo_cached() {
        let actual = ModuleRenderer::new("sudo")
            .cmd(
                "sudo -n true",
                Some(CommandOutput {
                    stdout: "".to_owned(),
                    stderr: "".to_owned(),
                }),
            )
            .config(toml::toml! {
                [sudo]
                disabled = false
                allow_windows = true