ronantakizawa/codereview-bench huggingface dataset:
Repository: casey/just
File: src/color.rs (Rust)

Code before:
    self.redirect(Stream::Stdout)
  }

  pub(crate) fn doc(self) -> Self {
    self.restyle(Style::new().fg(Blue))
  }

  pub(crate) fn error(self) -> Self {
    self.restyle(Style::new().fg(Red).bold())
  }

  pub(crate) fn warning(self) -> Self {
    self.restyle(Style::new().fg(Yellow).bold())
  }

  pub(crate) fn banner(self) -> Self {
    self.restyle(Style::new().fg(Cyan).bold())
  }

  pub(crate) fn command(self, foreground_color: Option<Colour>) -> Self {
    let new_style = Style {
      foreground: foreground_color,
      is_bold: true,
      ..Style::default()
    };
    self.restyle(new_style)
  }

  pub(crate) fn parameter(self) -> Self {
    self.restyle(Style::new().fg(Cyan))
  }

  pub(crate) fn message(self) -> Self {
    self.restyle(Style::new().bold())
  }

  pub(crate) fn annotation(self) -> Self {
    self.restyle(Style::new().fg(Purple))
  }

  pub(crate) fn string(self) -> Self {
    self.restyle(Style::new().fg(Green))
  }

  pub(crate) fn diff_added(self) -> Self {
    self.restyle(Style::new().fg(Green))
  }

  pub(crate) fn diff_deleted(self) -> Self {
    self.restyle(Style::new().fg(Red))
  }

Review comment (suggestion):
```suggestion
  pub(crate) fn command(self, foreground: Option<ansi_term::Color>) -> Self {
    self.restyle(Style {
      foreground,
      is_bold: true,
      ..Style::default()
    })
```

Code after:
  }

  pub(crate) fn doc(self) -> Self {
    self.restyle(Style::new().fg(Blue))
  }

  pub(crate) fn error(self) -> Self {
    self.restyle(Style::new().fg(Red).bold())
  }

  pub(crate) fn warning(self) -> Self {
    self.restyle(Style::new().fg(Yellow).bold())
  }

  pub(crate) fn banner(self) -> Self {
    self.restyle(Style::new().fg(Cyan).bold())
  }

  pub(crate) fn command(self, foreground: Option<ansi_term::Color>) -> Self {
    self.restyle(Style {
      foreground,
      is_bold: true,
      ..Style::default()
    })
  }

  pub(crate) fn parameter(self) -> Self {
    self.restyle(Style::new().fg(Cyan))
  }

  pub(crate) fn message(self) -> Self {
    self.restyle(Style::new().bold())
  }

  pub(crate) fn annotation(self) -> Self {
    self.restyle(Style::new().fg(Purple))
  }

  pub(crate) fn string(self) -> Self {
    self.restyle(Style::new().fg(Green))
  }

  pub(crate) fn diff_added(self) -> Self {
    self.restyle(Style::new().fg(Green))
  }

  pub(crate) fn diff_deleted(self) -> Self {
    self.restyle(Style::new().fg(Red))
  }

  pub(crate) fn active(&self) -> bool {
Repository: casey/just
File: src/attribute.rs (Rust)

Code before:
  Private,
  NoQuiet,
  Unix,
  Windows,
}

impl Attribute {
  pub(crate) fn from_name(name: Name) -> Option<Attribute> {
    name.lexeme().parse().ok()
  }

  pub(crate) fn to_str(self) -> &'static str {
    self.into()
  }
}

#[cfg(test)]
mod tests {
  use super::*;

  #[test]
  fn no_exit_message_to_str() {
    assert_eq!(Attribute::NoExitMessage.to_str(), "no-exit-message");
  }

  #[test]
  fn no_quiet_to_str() {
    assert_eq!(Attribute::NoQuiet.to_str(), "no-quiet");
  }
}


Review comment (refactor):
We can remove this test. The other test is there just to make sure that we're serializing attribute names in kebab-case, but we don't need to test every variant.

Code after:
  Private,
  NoQuiet,
  Unix,
  Windows,
}

impl Attribute {
  pub(crate) fn from_name(name: Name) -> Option<Attribute> {
    name.lexeme().parse().ok()
  }

  pub(crate) fn to_str(self) -> &'static str {
    self.into()
  }
}

#[cfg(test)]
mod tests {
  use super::*;

  #[test]
  fn to_str() {
    assert_eq!(Attribute::NoExitMessage.to_str(), "no-exit-message");
  }
}
Repository: casey/just
File: src/recipe.rs (Rust)

Code before:
  }

  pub(crate) fn change_directory(&self) -> bool {
    !self.attributes.contains(&Attribute::NoCd)
  }

  pub(crate) fn enabled(&self) -> bool {
    let windows = self.attributes.contains(&Attribute::Windows);
    let linux = self.attributes.contains(&Attribute::Linux);
    let macos = self.attributes.contains(&Attribute::Macos);
    let unix = self.attributes.contains(&Attribute::Unix);

    (!windows && !linux && !macos && !unix)
      || (cfg!(target_os = "windows") && windows)
      || (cfg!(target_os = "linux") && (linux || unix))
      || (cfg!(target_os = "macos") && (macos || unix))
      || (cfg!(windows) && windows)
      || (cfg!(unix) && unix)
  }

  fn print_exit_message(&self) -> bool {
    !self.attributes.contains(&Attribute::NoExitMessage)
  }

  fn no_quiet(&self) -> bool {
    !self.attributes.contains(&Attribute::NoQuiet)
  }

  pub(crate) fn run<'run>(
    &self,
    context: &RecipeContext<'src, 'run>,
    dotenv: &BTreeMap<String, String>,
    scope: Scope<'src, 'run>,
    search: &'run Search,
    positional: &[String],
  ) -> RunResult<'src, ()> {
    let config = &context.config;

    if config.verbosity.loquacious() {
      let color = config.color.stderr().banner();
      eprintln!(
        "{}===> Running recipe `{}`...{}",
        color.prefix(),
        self.name,
        color.suffix()
      );
    }

    let evaluator =
      Evaluator::recipe_evaluator(context.config, dotenv, &scope, context.settings, search);


Review comment (suggestion):
Should this be inverted, i.e.:

```suggestion
    self.attributes.contains(&Attribute::NoQuiet)
```

Code after:
    } else {
      Ok(true)
    }
  }

  pub(crate) fn check_can_be_default_recipe(&self) -> RunResult<'src, ()> {
    let min_arguments = self.min_arguments();
    if min_arguments > 0 {
      return Err(Error::DefaultRecipeRequiresArguments {
        recipe: self.name.lexeme(),
        min_arguments,
      });
    }

    Ok(())
  }

  pub(crate) fn is_public(&self) -> bool {
    !self.private && !self.attributes.contains(&Attribute::Private)
  }

  pub(crate) fn change_directory(&self) -> bool {
    !self.attributes.contains(&Attribute::NoCd)
  }

  pub(crate) fn enabled(&self) -> bool {
    let windows = self.attributes.contains(&Attribute::Windows);
    let linux = self.attributes.contains(&Attribute::Linux);
    let macos = self.attributes.contains(&Attribute::Macos);
    let unix = self.attributes.contains(&Attribute::Unix);

    (!windows && !linux && !macos && !unix)
      || (cfg!(target_os = "windows") && windows)
      || (cfg!(target_os = "linux") && (linux || unix))
      || (cfg!(target_os = "macos") && (macos || unix))
      || (cfg!(windows) && windows)
      || (cfg!(unix) && unix)
  }

  fn print_exit_message(&self) -> bool {
    !self.attributes.contains(&Attribute::NoExitMessage)
  }

  fn working_directory<'a>(&'a self, search: &'a Search) -> Option<&Path> {
    if self.change_directory() {
      Some(if self.depth > 0 {
        &self.working_directory
      } else {
        &search.working_directory
      })
    } else {
Repository: casey/just
File: src/recipe.rs (Rust)

Code before:
    if self.shebang {
      self.run_shebang(context, dotenv, &scope, positional, config, evaluator)
    } else {
      self.run_linewise(context, dotenv, &scope, positional, config, evaluator)
    }
  }

  fn run_linewise<'run>(
    &self,
    context: &RecipeContext<'src, 'run>,
    dotenv: &BTreeMap<String, String>,
    scope: &Scope<'src, 'run>,
    positional: &[String],
    config: &Config,
    mut evaluator: Evaluator<'src, 'run>,
  ) -> RunResult<'src, ()> {
    let mut lines = self.body.iter().peekable();
    let mut line_number = self.line_number() + 1;
    loop {
      if lines.peek().is_none() {
        return Ok(());
      }
      let mut evaluated = String::new();
      let mut continued = false;

      let quiet_command = self.no_quiet() && context.settings.quiet
        || lines.peek().map_or(false, |line| line.is_quiet());

      let infallible_command = lines.peek().map_or(false, |line| line.is_infallible());

      let comment_line =
        context.settings.ignore_comments && lines.peek().map_or(false, |line| line.is_comment());

      loop {
        if lines.peek().is_none() {
          break;
        }
        let line = lines.next().unwrap();
        line_number += 1;
        if !comment_line {
          evaluated += &evaluator.evaluate_line(line, continued)?;
        }
        if line.is_continuation() && !comment_line {
          continued = true;
          evaluated.pop();
        } else {
          break;
        }
      }

      if comment_line {

Review comment (suggestion):
This is used to determine if we should strip a leading `@` from the line, so we should only do so if it starts with an `@`:

```suggestion
      let quiet_line = lines.peek().map_or(false, |line| line.is_quiet());
```

Code after:
      None
    }
  }

  fn no_quiet(&self) -> bool {
    self.attributes.contains(&Attribute::NoQuiet)
  }

  pub(crate) fn run<'run>(
    &self,
    context: &RecipeContext<'src, 'run>,
    dotenv: &BTreeMap<String, String>,
    scope: Scope<'src, 'run>,
    search: &'run Search,
    positional: &[String],
  ) -> RunResult<'src, ()> {
    let config = &context.config;

    if config.verbosity.loquacious() {
      let color = config.color.stderr().banner();
      eprintln!(
        "{}===> Running recipe `{}`...{}",
        color.prefix(),
        self.name,
        color.suffix()
      );
    }

    let evaluator =
      Evaluator::recipe_evaluator(context.config, dotenv, &scope, context.settings, search);

    if self.shebang {
      self.run_shebang(context, dotenv, &scope, positional, config, evaluator)
    } else {
      self.run_linewise(context, dotenv, &scope, positional, config, evaluator)
    }
  }

  fn run_linewise<'run>(
    &self,
    context: &RecipeContext<'src, 'run>,
    dotenv: &BTreeMap<String, String>,
    scope: &Scope<'src, 'run>,
    positional: &[String],
    config: &Config,
    mut evaluator: Evaluator<'src, 'run>,
  ) -> RunResult<'src, ()> {
    let mut lines = self.body.iter().peekable();
    let mut line_number = self.line_number() + 1;
    loop {
      if lines.peek().is_none() {
Repository: casey/just
File: src/recipe.rs (Rust)

Code before:
        } else {
          config.color
        };
        eprintln!("{}", color.stderr().paint(command));
      }

      if config.dry_run {
        continue;
      }

      let mut cmd = context.settings.shell_command(config);

      if self.change_directory() {
        cmd.current_dir(&context.search.working_directory);
      }

      cmd.arg(command);

      if context.settings.positional_arguments {
        cmd.arg(self.name.lexeme());
        cmd.args(positional);
      }

      if config.verbosity.quiet() {
        cmd.stderr(Stdio::null());
        cmd.stdout(Stdio::null());
      }

      cmd.export(context.settings, dotenv, scope);

      match InterruptHandler::guard(|| cmd.status()) {
        Ok(exit_status) => {
          if let Some(code) = exit_status.code() {
            if code != 0 && !infallible_command {
              return Err(Error::Code {
                recipe: self.name(),
                line_number: Some(line_number),
                code,
                print_message: self.print_exit_message(),
              });
            }
          } else {
            return Err(error_from_signal(
              self.name(),
              Some(line_number),
              exit_status,
            ));
          }
        }
        Err(io_error) => {
          return Err(Error::Io {

Review comment (suggestion):
I think this should be something like:

```suggestion
        || !((quiet_line ^ self.quiet) || (context.settings.quiet && !self.no_quiet()) || config.verbosity.quiet())
```


Code after:
      }

      if comment_line {
        continue;
      }

      let mut command = evaluated.as_str();

      let sigils = usize::from(infallible_line) + usize::from(quiet_line);

      command = &command[sigils..];

      if command.is_empty() {
        continue;
      }

      if config.dry_run
        || config.verbosity.loquacious()
        || !((quiet_line ^ self.quiet)
          || (context.settings.quiet && !self.no_quiet())
          || config.verbosity.quiet())
      {
        let color = if config.highlight {
          config.color.command(config.command_color)
        } else {
          config.color
        };
        eprintln!("{}", color.stderr().paint(command));
      }

      if config.dry_run {
        continue;
      }

      let mut cmd = context.settings.shell_command(config);

      if let Some(working_directory) = self.working_directory(context.search) {
        cmd.current_dir(working_directory);
      }

      cmd.arg(command);

      if context.settings.positional_arguments {
        cmd.arg(self.name.lexeme());
        cmd.args(positional);
      }

      if config.verbosity.quiet() {
        cmd.stderr(Stdio::null());
        cmd.stdout(Stdio::null());
      }
Repository: casey/just
File: src/function.rs (Rust)

Code before:
use semver::VersionReq;

use {
  super::*,
  heck::{
    ToKebabCase, ToLowerCamelCase, ToShoutyKebabCase, ToShoutySnakeCase, ToSnakeCase, ToTitleCase,
    ToUpperCamelCase,
  },
  semver::Version,
  Function::*,
};

pub(crate) enum Function {
  Nullary(fn(&FunctionContext) -> Result<String, String>),
  Unary(fn(&FunctionContext, &str) -> Result<String, String>),
  UnaryOpt(fn(&FunctionContext, &str, Option<&str>) -> Result<String, String>),
  Binary(fn(&FunctionContext, &str, &str) -> Result<String, String>),
  BinaryPlus(fn(&FunctionContext, &str, &str, &[String]) -> Result<String, String>),
  Ternary(fn(&FunctionContext, &str, &str, &str) -> Result<String, String>),
}

pub(crate) fn get(name: &str) -> Option<Function> {
  let function = match name {
    "absolute_path" => Unary(absolute_path),
    "arch" => Nullary(arch),
    "capitalize" => Unary(capitalize),
    "clean" => Unary(clean),
    "env" => UnaryOpt(env),
    "env_var" => Unary(env_var),
    "env_var_or_default" => Binary(env_var_or_default),
    "error" => Unary(error),
    "extension" => Unary(extension),
    "file_name" => Unary(file_name),
    "file_stem" => Unary(file_stem),

Review comment (suggestion):
You can put everything into the same use block:

```suggestion
use {
  super::*,
  heck::{
    ToKebabCase, ToLowerCamelCase, ToShoutyKebabCase, ToShoutySnakeCase, ToSnakeCase, ToTitleCase,
    ToUpperCamelCase,
  },
  semver::{Version, VersionReq},
```

Code after:
use {
  super::*,
  heck::{
    ToKebabCase, ToLowerCamelCase, ToShoutyKebabCase, ToShoutySnakeCase, ToSnakeCase, ToTitleCase,
    ToUpperCamelCase,
  },
  semver::{Version, VersionReq},
  Function::*,
};

pub(crate) enum Function {
  Nullary(fn(&FunctionContext) -> Result<String, String>),
  Unary(fn(&FunctionContext, &str) -> Result<String, String>),
  UnaryOpt(fn(&FunctionContext, &str, Option<&str>) -> Result<String, String>),
  Binary(fn(&FunctionContext, &str, &str) -> Result<String, String>),
  BinaryPlus(fn(&FunctionContext, &str, &str, &[String]) -> Result<String, String>),
  Ternary(fn(&FunctionContext, &str, &str, &str) -> Result<String, String>),
}

pub(crate) fn get(name: &str) -> Option<Function> {
  let function = match name {
    "absolute_path" => Unary(absolute_path),
    "arch" => Nullary(arch),
    "capitalize" => Unary(capitalize),
    "clean" => Unary(clean),
    "env" => UnaryOpt(env),
    "env_var" => Unary(env_var),
    "env_var_or_default" => Binary(env_var_or_default),
    "error" => Unary(error),
    "extension" => Unary(extension),
    "file_name" => Unary(file_name),
    "file_stem" => Unary(file_stem),
    "invocation_directory" => Nullary(invocation_directory),
    "invocation_directory_native" => Nullary(invocation_directory_native),
Repository: casey/just
File: src/function.rs (Rust)

Code before:
fn uppercase(_context: &FunctionContext, s: &str) -> Result<String, String> {
  Ok(s.to_uppercase())
}

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(_context: &FunctionContext, v: &str, req: &str) -> Result<String, String> {
  Ok(
    VersionReq::parse(req.as_ref())
      .map_err(|err| err.to_string())?
      .matches(&Version::parse(v.as_ref()).map_err(|err| err.to_string())?)
      .to_string(),
  )
}


Review comment (suggestion):
I'm curious what the error message looks like. Sometimes libraries produce errors which aren't super helpful when they're bubbled up to the end user. If it's ambiguous or doesn't give enough information, add context with something like:

```suggestion
      .map_err(|err| format!{"invalid semver: {err}")?
```

Code after:
}

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(
  _context: &FunctionContext,
  version: &str,
  requirement: &str,
) -> Result<String, String> {
  Ok(
    requirement
      .parse::<VersionReq>()
      .map_err(|err| format!("invalid semver requirement: {err}"))?
      .matches(
        &version
          .parse::<Version>()
          .map_err(|err| format!("invalid semver version: {err}"))?,
      )
      .to_string(),
  )
}
Repository: casey/just
File: src/function.rs (Rust)

Code before:

fn uppercase(_context: &FunctionContext, s: &str) -> Result<String, String> {
  Ok(s.to_uppercase())
}

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(_context: &FunctionContext, v: &str, req: &str) -> Result<String, String> {
  Ok(
    VersionReq::parse(req.as_ref())
      .map_err(|err| err.to_string())?
      .matches(&Version::parse(v.as_ref()).map_err(|err| err.to_string())?)
      .to_string(),
  )
}


Review comment (suggestion):
I like using parse with a turbofish:

```suggestion
    req.parse::<VersionReq>()
```

Code after:
  Ok(s.to_uppercase())
}

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(
  _context: &FunctionContext,
  version: &str,
  requirement: &str,
) -> Result<String, String> {
  Ok(
    requirement
      .parse::<VersionReq>()
      .map_err(|err| format!("invalid semver requirement: {err}"))?
      .matches(
        &version
          .parse::<Version>()
          .map_err(|err| format!("invalid semver version: {err}"))?,
      )
      .to_string(),
  )
}
Repository: casey/just
File: src/function.rs (Rust)

Code before:
  Ok(s.to_uppercase())
}

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(_context: &FunctionContext, v: &str, req: &str) -> Result<String, String> {
  Ok(
    VersionReq::parse(req.as_ref())
      .map_err(|err| err.to_string())?
      .matches(&Version::parse(v.as_ref()).map_err(|err| err.to_string())?)
      .to_string(),
  )
}


Review comment (suggestion):
Not sure if it works with the type inference here, but you might be able to do:

```suggestion
      .matches(&v.parse().map_err(|err| err.to_string())?)
```

The same comment about the error applies here. If it's ambiguous on it's own, give it some context:

Code after:

fn uuid(_context: &FunctionContext) -> Result<String, String> {
  Ok(uuid::Uuid::new_v4().to_string())
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{path}`"))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{path}`"))?;

  Ok(parent.join(file_stem).to_string())
}

/// Check whether a string processes properly as semver (e.x. "0.1.0")
/// and matches a given semver requirement (e.x. ">=0.1.0")
fn semver_matches(
  _context: &FunctionContext,
  version: &str,
  requirement: &str,
) -> Result<String, String> {
  Ok(
    requirement
      .parse::<VersionReq>()
      .map_err(|err| format!("invalid semver requirement: {err}"))?
      .matches(
        &version
          .parse::<Version>()
          .map_err(|err| format!("invalid semver version: {err}"))?,
      )
      .to_string(),
  )
}