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

Code before:
  pub(crate) fn min_arguments(&self) -> usize {
    self
      .parameters
      .iter()
      .filter(|p| p.default.is_none() && p.kind != ParameterKind::Star)
      .count()
  }

  pub(crate) fn max_arguments(&self) -> usize {
    if self.parameters.iter().any(|p| p.kind.is_variadic()) {
      usize::MAX - 1
    } else {
      self.parameters.len()
    }
  }

  pub(crate) fn name(&self) -> &'src str {
    self.name.lexeme()
  }

  pub(crate) fn line_number(&self) -> usize {
    self.name.line
  }

  pub(crate) fn confirm(&self) -> RunResult<'src, bool> {
    if let Some(Attribute::Confirm(prompt_text)) = self
      .attributes
      .iter()
      .find(|attr| matches!(attr, Attribute::Confirm(_)))
    {
      let optional_prompt_text: Box<str> = prompt_text
        .as_ref()
        .map_or(String::new().into_boxed_str(), |s| format!("{s} - ").into());
      eprint!("{optional_prompt_text}Run recipe `{}`? ", self.name,);
      let mut line = String::new();
      std::io::stdin()
        .read_line(&mut line)
        .map_err(|io_error| Error::GetConfirmation { io_error })?;
      let line = line.trim().to_lowercase();
      Ok(line == "y" || line == "yes")
    } 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,

Review comment (suggestion):
Let's do something like:

```
for attribute in self.attributes {
  if let Attribute::Confirm(prompt) = attribute {
    // print prompt
    break;
  }
}
``

Code after:
  }

  pub(crate) fn min_arguments(&self) -> usize {
    self
      .parameters
      .iter()
      .filter(|p| p.default.is_none() && p.kind != ParameterKind::Star)
      .count()
  }

  pub(crate) fn max_arguments(&self) -> usize {
    if self.parameters.iter().any(|p| p.kind.is_variadic()) {
      usize::MAX - 1
    } else {
      self.parameters.len()
    }
  }

  pub(crate) fn name(&self) -> &'src str {
    self.name.lexeme()
  }

  pub(crate) fn line_number(&self) -> usize {
    self.name.line
  }

  pub(crate) fn confirm(&self) -> RunResult<'src, bool> {
    for attribute in &self.attributes {
      if let Attribute::Confirm(prompt) = attribute {
        if let Some(prompt) = prompt {
          eprint!("{} ", prompt.cooked);
        } else {
          eprint!("Run recipe `{}`? ", self.name);
        }
        let mut line = String::new();
        std::io::stdin()
          .read_line(&mut line)
          .map_err(|io_error| Error::GetConfirmation { io_error })?;
        let line = line.trim().to_lowercase();
        return Ok(line == "y" || line == "yes");
      }
    }
    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,
Repository: casey/just
File: src/parser.rs (Rust)

Code before:
    Ok(Shell { arguments, command })
  }

  /// Parse recipe attributes
  fn parse_attributes(&mut self) -> CompileResult<'src, Option<BTreeSet<Attribute>>> {
    let mut attributes = BTreeMap::new();

    while self.accepted(BracketL)? {
      loop {
        let name = self.parse_name()?;
        let attribute = Attribute::from_name(name).ok_or_else(|| {
          name.error(CompileErrorKind::UnknownAttribute {
            attribute: name.lexeme(),
          })
        })?;
        if let Some(line) = attributes.get(&attribute) {
          return Err(name.error(CompileErrorKind::DuplicateAttribute {
            attribute: name.lexeme(),
            first: *line,
          }));
        }

        let arguments = if self.accepted(ParenL)? {
          let mut arguments = Vec::new();

          while self.next_is(StringToken) {
            arguments.push(self.parse_string_literal()?);
          }
          self.expect(ParenR)?;
          Some(arguments)
        } else {
          None
        };

        let attribute = if let Some(arguments) = arguments {
          match attribute.with_arguments(arguments) {
            Ok(attribute) => attribute,
            Err(kind) =>  {
              return Err(name.error(kind));
            },
          }
        } else {
          attribute
        };

        attributes.insert(attribute, name.line);


        if !self.accepted(Comma)? {
          break;
        }

Review comment (suggestion):
Actually, we only have one attribute that accepts a string literal, and if we're accepting multiple arguments, we would want to accept commas between them, so we can make `arguments` be an `Option`.

Code after:
    Ok(Shell { arguments, command })
  }

  /// Parse recipe attributes
  fn parse_attributes(&mut self) -> CompileResult<'src, Option<BTreeSet<Attribute<'src>>>> {
    let mut attributes = BTreeMap::new();

    while self.accepted(BracketL)? {
      loop {
        let name = self.parse_name()?;
        let attribute = Attribute::from_name(name).ok_or_else(|| {
          name.error(CompileErrorKind::UnknownAttribute {
            attribute: name.lexeme(),
          })
        })?;
        if let Some(line) = attributes.get(&attribute) {
          return Err(name.error(CompileErrorKind::DuplicateAttribute {
            attribute: name.lexeme(),
            first: *line,
          }));
        }

        let attribute = if self.accepted(ParenL)? {
          let argument = self.parse_string_literal()?;
          self.expect(ParenR)?;
          attribute.with_argument(name, argument)?
        } else {
          attribute
        };

        attributes.insert(attribute, name.line);

        if !self.accepted(Comma)? {
          break;
        }
      }
      self.expect(BracketR)?;
      self.expect_eol()?;
    }

    if attributes.is_empty() {
      Ok(None)
    } else {
      Ok(Some(attributes.into_keys().collect()))
    }
  }
}

#[cfg(test)]
mod tests {
  use super::*;
Repository: casey/just
File: www/install.sh (Shell)

Code before:
}

err() {
  if [ -n "${td-}" ]; then
    rm -rf "$td"
  fi

  say "error: $*"
  exit 1
}

need() {
  if ! command -v "$1" > /dev/null 2>&1; then
    err "need $1 (command not found)"
  fi
}

download() {
  url="$1"
  output="$2"

  # use curl if available, wget otherwise
  if command -v curl > /dev/null; then
    curl --proto =https --tlsv1.2 -sSfL "$url" "-o$output"
  else
    # Some wget implementations support these flags, but busybox
    # (used by alpine) does not. Also busybox help uses stderr
    # while gnu uses stdout...
    shift "$#" # clear function arguments to use as an array
    wget --help 2>&1 | grep -q https-only && set -- "$@" "--https-only"
    wget --help 2>&1 | grep -q secure-protocol &&
      set -- "$@" "--secure-protocol=TLSv1_3"
    wget --help 2>&1 | grep -q no-verbose && quiet="--no-verbose"
    set -- "$@" "${quiet:---quiet}"
    wget "$@" "$url" "-O$output"
  fi
}

force=false
while test $# -gt 0; do
  case $1 in
    --force | -f)
      force=true
      ;;
    --help | -h)
      help
      exit 0
      ;;
    --tag)
      tag=$2
      shift

Review comment (bug):
How about trying `wget` with all options and then falling back to using none of the options if that fails? We might try the download twice, if there's some other error, like the network not being up, but that might be better than checking each flag individually?

Code after:
}

err() {
  if [ -n "${td-}" ]; then
    rm -rf "$td"
  fi

  say "error: $*"
  exit 1
}

need() {
  if ! command -v "$1" > /dev/null 2>&1; then
    err "need $1 (command not found)"
  fi
}

download() {
  url="$1"
  output="$2"

  if command -v curl > /dev/null; then
    curl --proto =https --tlsv1.2 -sSfL "$url" "-o$output"
  else
    wget --https-only --secure-protocol=TLSv1_2 --quiet "$url" "-O$output"
  fi
}

force=false
while test $# -gt 0; do
  case $1 in
    --force | -f)
      force=true
      ;;
    --help | -h)
      help
      exit 0
      ;;
    --tag)
      tag=$2
      shift
      ;;
    --target)
      target=$2
      shift
      ;;
    --to)
      dest=$2
      shift
      ;;
    *)
Repository: casey/just
File: src/function.rs (Rust)

Code before:
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn pick(_evaluator: &Evaluator, count: &str, alphabet: Option<&str>) -> Result<String, String> {
  let alphabet = alphabet.unwrap_or("0123456789abcdef");
  if alphabet.is_empty() {
    return Err(String::from("no characters in alphabet"));
  }
  let mut alphabet_unique = HashSet::<char>::with_capacity(alphabet.len());
  for c in alphabet.chars() {
    if !alphabet_unique.insert(c) {
      return Err(format!("alphabet contains repeated character {c}"));
    }
  }
  let alphabet = alphabet_unique.drain().collect::<Vec<char>>();
  let len: usize = match count.parse::<usize>() {
    Ok(l) => l,
    _ => return Err(format!("Could not parse `{count}` as a positive integer")),
  };
  let mut rng = thread_rng();
  let mut r = String::with_capacity(len);
  while r.len() < len {
    r.push(*alphabet.choose(&mut rng).unwrap());
  }
  Ok(r)
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}

Review comment (refactor):
let's move this out to a `const` at the top -- the default alphabet should probably be called out (and possibly reusable) somewhere.

Code after:
        justfile_directory.display()
      )
    })
}

fn kebabcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_kebab_case())
}

fn lowercamelcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lower_camel_case())
}

fn lowercase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lowercase())
}

fn num_cpus(_evaluator: &Evaluator) -> Result<String, String> {
  let num = num_cpus::get();
  Ok(num.to_string())
}

fn os(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::os().to_owned())
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}
Repository: casey/just
File: src/function.rs (Rust)

Code before:
fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn pick(_evaluator: &Evaluator, count: &str, alphabet: Option<&str>) -> Result<String, String> {
  let alphabet = alphabet.unwrap_or("0123456789abcdef");
  if alphabet.is_empty() {
    return Err(String::from("no characters in alphabet"));
  }
  let mut alphabet_unique = HashSet::<char>::with_capacity(alphabet.len());
  for c in alphabet.chars() {
    if !alphabet_unique.insert(c) {
      return Err(format!("alphabet contains repeated character {c}"));
    }
  }
  let alphabet = alphabet_unique.drain().collect::<Vec<char>>();
  let len: usize = match count.parse::<usize>() {
    Ok(l) => l,
    _ => return Err(format!("Could not parse `{count}` as a positive integer")),
  };
  let mut rng = thread_rng();
  let mut r = String::with_capacity(len);
  while r.len() < len {
    r.push(*alphabet.choose(&mut rng).unwrap());
  }
  Ok(r)
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}

fn replace(_evaluator: &Evaluator, s: &str, from: &str, to: &str) -> Result<String, String> {
  Ok(s.replace(from, to))
}

fn replace_regex(
  _evaluator: &Evaluator,
  s: &str,
  regex: &str,
  replacement: &str,
) -> Result<String, String> {
  Ok(
    Regex::new(regex)

Review comment (bug):
```suggestion
    Err(e) => return Err(format!("failed to parse `{count}` as a positive integer: {e}")),
```

Super glad you have the count that failed to parse here -- messages like this are missing the thing that failed to parse way too often in the wild!

Code after:
fn lowercase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lowercase())
}

fn num_cpus(_evaluator: &Evaluator) -> Result<String, String> {
  let num = num_cpus::get();
  Ok(num.to_string())
}

fn os(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::os().to_owned())
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}

fn replace(_evaluator: &Evaluator, s: &str, from: &str, to: &str) -> Result<String, String> {
  Ok(s.replace(from, to))
}

fn replace_regex(
  _evaluator: &Evaluator,
  s: &str,
  regex: &str,
  replacement: &str,
) -> Result<String, String> {
  Ok(
    Regex::new(regex)
Repository: casey/just
File: src/function.rs (Rust)

Code before:
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn pick(_evaluator: &Evaluator, count: &str, alphabet: Option<&str>) -> Result<String, String> {
  let alphabet = alphabet.unwrap_or(DEFAULT_RANDOM_ALPHABET);
  if alphabet.is_empty() {
    return Err(String::from("no characters in alphabet"));
  }
  let mut alphabet_unique = HashSet::<char>::with_capacity(alphabet.len());
  for c in alphabet.chars() {
    if !alphabet_unique.insert(c) {
      return Err(format!("alphabet contains repeated character `{c}`"));
    }
  }
  let alphabet = alphabet_unique.drain().collect::<Vec<char>>();
  let len: usize = match count.parse::<usize>() {
    Ok(l) => l,
    Err(e) => {
      return Err(format!(
        "failed to parse `{count}` as a positive integer: {e}"
      ));
    }
  };
  let mut rng = thread_rng();
  let mut r = String::with_capacity(len);
  while r.len() < len {
    r.push(*alphabet.choose(&mut rng).unwrap());
  }
  Ok(r)
}

Review comment (suggestion):
I prefer not to create constants that are only used in one place. This can just be `let alphabet = alphabet.unwrap_or("0123456789abcdef");`

Code after:
    })
}

fn kebabcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_kebab_case())
}

fn lowercamelcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lower_camel_case())
}

fn lowercase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lowercase())
}

fn num_cpus(_evaluator: &Evaluator) -> Result<String, String> {
  let num = num_cpus::get();
  Ok(num.to_string())
}

fn os(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::os().to_owned())
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}

fn replace(_evaluator: &Evaluator, s: &str, from: &str, to: &str) -> Result<String, String> {
Repository: casey/just
File: src/function.rs (Rust)

Code before:
fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn pick(_evaluator: &Evaluator, count: &str, alphabet: Option<&str>) -> Result<String, String> {
  let alphabet = alphabet.unwrap_or(DEFAULT_RANDOM_ALPHABET);
  if alphabet.is_empty() {
    return Err(String::from("no characters in alphabet"));
  }
  let mut alphabet_unique = HashSet::<char>::with_capacity(alphabet.len());
  for c in alphabet.chars() {
    if !alphabet_unique.insert(c) {
      return Err(format!("alphabet contains repeated character `{c}`"));
    }
  }
  let alphabet = alphabet_unique.drain().collect::<Vec<char>>();
  let len: usize = match count.parse::<usize>() {
    Ok(l) => l,
    Err(e) => {
      return Err(format!(
        "failed to parse `{count}` as a positive integer: {e}"
      ));
    }
  };
  let mut rng = thread_rng();
  let mut r = String::with_capacity(len);
  while r.len() < len {
    r.push(*alphabet.choose(&mut rng).unwrap());
  }
  Ok(r)
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {

Review comment (suggestion):
```suggestion
    return Err("alphabet is empty".into());
```

Code after:

fn kebabcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_kebab_case())
}

fn lowercamelcase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lower_camel_case())
}

fn lowercase(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(s.to_lowercase())
}

fn num_cpus(_evaluator: &Evaluator) -> Result<String, String> {
  let num = num_cpus::get();
  Ok(num.to_string())
}

fn os(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::os().to_owned())
}

fn os_family(_evaluator: &Evaluator) -> Result<String, String> {
  Ok(target::family().to_owned())
}

fn parent_directory(_evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{path}`"))
}

fn path_exists(evaluator: &Evaluator, path: &str) -> Result<String, String> {
  Ok(
    evaluator
      .search
      .working_directory
      .join(path)
      .exists()
      .to_string(),
  )
}

fn quote(_evaluator: &Evaluator, s: &str) -> Result<String, String> {
  Ok(format!("'{}'", s.replace('\'', "'\\''")))
}

fn replace(_evaluator: &Evaluator, s: &str, from: &str, to: &str) -> Result<String, String> {
  Ok(s.replace(from, to))
}
Repository: casey/just
File: www/install.sh (Shell)

Code before:
    --help | -h)
      help
      exit 0
      ;;
    --tag)
      tag=$2
      shift
      ;;
    --target)
      target=$2
      shift
      ;;
    --to)
      dest=$2
      shift
      ;;
    *)
      say "error: unrecognized argument '$1'. Usage:"
      help
      exit 1
      ;;
  esac
  shift
done

need_one_of curl wget
need_one_of tar unzip
need mkdir
need mktemp

if [ -z "${tag-}" ]; then
  need grep
  need cut
fi

if [ -z "${target-}" ]; then
  need cut
fi

if [ -z "${dest-}" ]; then
  dest="$HOME/bin"
fi

if [ -z "${tag-}" ]; then
  tag=$(
    download https://api.github.com/repos/casey/just/releases/latest - |
    grep tag_name |
    cut -d'"' -f4
  )
fi


Review comment (suggestion):
Doing `need unzip` or `need tar` in the `$target` case statement seems fine to me, instead of adding a `need_one_of` function.

Code after:

command -v curl > /dev/null 2>&1 ||
  command -v wget > /dev/null 2>&1 ||
  err "need wget or curl (command not found)"

need mkdir
need mktemp

if [ -z "${tag-}" ]; then
  need grep
  need cut
fi

if [ -z "${target-}" ]; then
  need cut
fi

if [ -z "${dest-}" ]; then
  dest="$HOME/bin"
fi

if [ -z "${tag-}" ]; then
  tag=$(
    download https://api.github.com/repos/casey/just/releases/latest - |
    grep tag_name |
    cut -d'"' -f4
  )
fi

if [ -z "${target-}" ]; then
  # bash compiled with MINGW (e.g. git-bash, used in github windows runners),
  # unhelpfully includes a version suffix in `uname -s` output, so handle that.
  # e.g. MINGW64_NT-10-0.19044
  kernel=$(uname -s | cut -d- -f1)
  uname_target="$(uname -m)-$kernel"

  case $uname_target in
    aarch64-Linux) target=aarch64-unknown-linux-musl;;
    arm64-Darwin) target=aarch64-apple-darwin;;
    armv6l-Linux) target=arm-unknown-linux-musleabihf;;
    armv7l-Linux) target=armv7-unknown-linux-musleabihf;;
    x86_64-Darwin) target=x86_64-apple-darwin;;
    x86_64-Linux) target=x86_64-unknown-linux-musl;;
    x86_64-MINGW64_NT) target=x86_64-pc-windows-msvc;;
    x86_64-Windows_NT) target=x86_64-pc-windows-msvc;;
    *)
      # shellcheck disable=SC2016
      err 'Could not determine target from output of `uname -m`-`uname -s`, please use `--target`:' "$uname_target"
    ;;
  esac
fi