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

Code before:
    .justfile("x := error ('Thing Not Supported')")
    .args(&["--evaluate"])
    .status(1)
    .stderr("error: Call to function `error` failed: Thing Not Supported\n  |\n1 | x := error ('Thing Not Supported')\n  |      ^^^^^\n")
    .run();
}

#[test]
fn error_errors_with_no_message() {
  Test::new()
    .justfile("x := error ('')")
    .args(&["--evaluate"])
    .status(1)
    .stderr("error: Call to function `error` failed: \n  |\n1 | x := error ('')\n  |      ^^^^^\n")
    .run();
}

#[test]
fn error_errors_with_message_based_upon_branch() {
  Test::new()
    .justfile("x := if \"true\" == \"false\" {{ error(\"if case\") }} else {{ error(\"else case\") }}")
    .args(&["--evaluate"])
    .status(1)
    .stderr("error: Expected backtick, identifier, '(', or string, but found '{'\n  |\n1 | x := if \"true\" == \"false\" {{ error(\"if case\") }} else {{ error(\"else case\") }}\n  |                            ^\n")
    .run();
}


Review comment (suggestion):
I think these tests are actually not required, since they don't exercise a different codepath in the PR. In other words, the PR could not have introduced a but which these tests could catch.

```suggestion
```

Code after:
    .justfile("x := error ('Thing Not Supported')")
    .args(&["--evaluate"])
    .status(1)
    .stderr("error: Call to function `error` failed: Thing Not Supported\n  |\n1 | x := error ('Thing Not Supported')\n  |      ^^^^^\n")
    .run();
}

#[test]
fn test_absolute_path_resolves() {
  let test_object = Test::new()
    .justfile("path := absolute_path('./test_file')")
    .args(&["--evaluate", "path"]);

  let mut tempdir = test_object.tempdir.path().to_owned();

  // Just retrieves the current directory via env::current_dir(), which
  // does the moral equivalent of canonicalize, which will remove symlinks.
  // So, we have to canonicalize here, so that we can match it.
  if cfg!(unix) {
    tempdir = tempdir.canonicalize().unwrap();
  }

  test_object
    .stdout(tempdir.join("test_file").to_str().unwrap().to_owned())
    .run();
}

#[test]
fn test_absolute_path_resolves_parent() {
  let test_object = Test::new()
    .justfile("path := absolute_path('../test_file')")
    .args(&["--evaluate", "path"]);

  let mut tempdir = test_object.tempdir.path().to_owned();

  // Just retrieves the current directory via env::current_dir(), which
  // does the moral equivalent of canonicalize, which will remove symlinks.
  // So, we have to canonicalize here, so that we can match it.
  if cfg!(unix) {
    tempdir = tempdir.canonicalize().unwrap();
  }

  test_object
    .stdout(
      tempdir
        .parent()
        .unwrap()
        .join("test_file")
        .to_str()
        .unwrap()
        .to_owned(),
Repository: casey/just
File: tests/functions.rs (Rust)

Code before:
}

#[test]
fn test_path_exists_filepath_exist() {
  Test::new()
    .tree(tree! {
      testfile: ""
    })
    .justfile("x := path_exists('testfile')")
    .args(&["--evaluate", "x"])
    .stdout("true")
    .run();
}

#[test]
fn test_path_exists_filepath_doesnt_exist() {
  Test::new()
    .justfile("x := path_exists('testfile')")
    .args(&["--evaluate", "x"])
    .stdout("false")
    .run();
}

#[test]
fn test_absolute_path_resolves() {
  let test_object = Test::new()
    .justfile("path := absolute_path('./test_file')")
    .args(&["--evaluate", "path"]);

  let tempdir = test_object.tempdir.path().to_owned();

  test_object
    .stdout(tempdir.join("test_file").to_str().unwrap().to_owned())
    .run();
}

#[test]
fn test_absolute_path_resolves_parent() {
  let test_object = Test::new()
    .justfile("path := absolute_path('../test_file')")
    .args(&["--evaluate", "path"]);

  let tempdir = test_object.tempdir.path().to_owned();

  test_object
    .stdout(
      tempdir
        .parent()
        .unwrap()
        .join("test_file")
        .to_str()

Review comment (suggestion):
Take a look at other tests that use the `tree` function to populate the tmpdir with files. You should be able to use that to do everything in one expression.

Code after:
}

#[test]
fn test_path_exists_filepath_exist() {
  Test::new()
    .tree(tree! {
      testfile: ""
    })
    .justfile("x := path_exists('testfile')")
    .args(&["--evaluate", "x"])
    .stdout("true")
    .run();
}

#[test]
fn test_path_exists_filepath_doesnt_exist() {
  Test::new()
    .justfile("x := path_exists('testfile')")
    .args(&["--evaluate", "x"])
    .stdout("false")
    .run();
}

#[test]
fn test_absolute_path_resolves() {
  let test_object = Test::new()
    .justfile("path := absolute_path('./test_file')")
    .args(&["--evaluate", "path"]);

  let mut tempdir = test_object.tempdir.path().to_owned();

  // Just retrieves the current directory via env::current_dir(), which
  // does the moral equivalent of canonicalize, which will remove symlinks.
  // So, we have to canonicalize here, so that we can match it.
  if cfg!(unix) {
    tempdir = tempdir.canonicalize().unwrap();
  }

  test_object
    .stdout(tempdir.join("test_file").to_str().unwrap().to_owned())
    .run();
}

#[test]
fn test_absolute_path_resolves_parent() {
  let test_object = Test::new()
    .justfile("path := absolute_path('../test_file')")
    .args(&["--evaluate", "path"]);

  let mut tempdir = test_object.tempdir.path().to_owned();
Repository: casey/just
File: tests/functions.rs (Rust)

Code before:
  let tempdir = test_object.tempdir.path().to_owned();

  test_object
    .stdout(tempdir.join("test_file").to_str().unwrap().to_owned())
    .run();
}

#[test]
fn test_absolute_path_resolves_parent() {
  let test_object = Test::new()
    .justfile("path := absolute_path('../test_file')")
    .args(&["--evaluate", "path"]);

  let tempdir = test_object.tempdir.path().to_owned();

  test_object
    .stdout(
      tempdir
        .parent()
        .unwrap()
        .join("test_file")
        .to_str()
        .unwrap()
        .to_owned(),
    )
    master
    .run();
}

#[test]
fn path_exists_subdir() {
  Test::new()
    .tree(tree! {
      foo: "",
      bar: {
      }
    })
    .justfile("x := path_exists('foo')")
    .current_dir("bar")
    .args(&["--evaluate", "x"])
    .stdout("true")
    .run();
}


Review comment (suggestion):
Whoops! Added this while fixing a merge conflict.

```suggestion
```

Code after:
  let mut tempdir = test_object.tempdir.path().to_owned();

  // Just retrieves the current directory via env::current_dir(), which
  // does the moral equivalent of canonicalize, which will remove symlinks.
  // So, we have to canonicalize here, so that we can match it.
  if cfg!(unix) {
    tempdir = tempdir.canonicalize().unwrap();
  }

  test_object
    .stdout(tempdir.join("test_file").to_str().unwrap().to_owned())
    .run();
}

#[test]
fn test_absolute_path_resolves_parent() {
  let test_object = Test::new()
    .justfile("path := absolute_path('../test_file')")
    .args(&["--evaluate", "path"]);

  let mut tempdir = test_object.tempdir.path().to_owned();

  // Just retrieves the current directory via env::current_dir(), which
  // does the moral equivalent of canonicalize, which will remove symlinks.
  // So, we have to canonicalize here, so that we can match it.
  if cfg!(unix) {
    tempdir = tempdir.canonicalize().unwrap();
  }

  test_object
    .stdout(
      tempdir
        .parent()
        .unwrap()
        .join("test_file")
        .to_str()
        .unwrap()
        .to_owned(),
    )
    .run();
}

#[test]
fn path_exists_subdir() {
  Test::new()
    .tree(tree! {
      foo: "",
      bar: {
      }
    })
    .justfile("x := path_exists('foo')")
Repository: casey/just
File: tests/functions.rs (Rust)

Code before:
  Test::new()
    .justfile("x := uuid()")
    .args(&["--evaluate", "x"])
    .stdout_regex("........-....-....-....-............")
    .run();
}

#[test]
fn sha256() {
  Test::new()
    .justfile("x := sha256('5943ee37-0000-1000-8000-010203040506')")
    .args(&["--evaluate", "x"])
    .stdout("2330d7f5eb94a820b54fed59a8eced236f80b633a504289c030b6a65aef58871")
    .run();
}

#[test]
fn sha256_file() {
  let mut tmpfile = tempfile::NamedTempFile::new().unwrap();
  writeln!(tmpfile, "just is great").unwrap();
  let tmppath = tmpfile.into_temp_path();

  let justfile_content = format!("x := sha256_file('{}')", tmppath.to_str().unwrap());

  Test::new()
    .justfile(justfile_content)
    .args(&["--evaluate", "x"])
    .stdout("177b3d79aaafb53a7a4d7aaba99a82f27c73370e8cb0295571aade1e4fea1cd2")
    .run();
}


Review comment (suggestion):
You can use the somewhat odd `tree` function to populate the tempdir with files:

```suggestion
    .justfile("x := sha256_file('file')")
    .tree(tree! {
      file: "just is great",
    })
```

Code after:
  Test::new()
    .justfile("x := uuid()")
    .args(&["--evaluate", "x"])
    .stdout_regex("........-....-....-....-............")
    .run();
}

#[test]
fn sha256() {
  Test::new()
    .justfile("x := sha256('5943ee37-0000-1000-8000-010203040506')")
    .args(&["--evaluate", "x"])
    .stdout("2330d7f5eb94a820b54fed59a8eced236f80b633a504289c030b6a65aef58871")
    .run();
}

#[test]
fn sha256_file() {
  Test::new()
    .justfile("x := sha256_file('sub/shafile')")
    .tree(tree! {
      sub: {
        shafile: "just is great\n",
      }
    })
    .current_dir("sub")
    .args(&["--evaluate", "x"])
    .stdout("177b3d79aaafb53a7a4d7aaba99a82f27c73370e8cb0295571aade1e4fea1cd2")
    .run();
}
Repository: casey/just
File: src/compile_error_kind.rs (Rust)

Code before:
  },
  InconsistentLeadingWhitespace {
    expected: &'src str,
    found: &'src str,
  },
  Internal {
    message: String,
  },
  InvalidEscapeSequence {
    character: char,
  },
  MismatchedClosingDelimiter {
    close: Delimiter,
    open: Delimiter,
    open_line: usize,
  },
  MixedLeadingWhitespace {
    whitespace: &'src str,
  },
  ParameterFollowsVariadicParameter {
    parameter: &'src str,
  },
  ParameterShadowsVariable {
    parameter: &'src str,
  },
  RecursionDepthExceeded,
  RequiredParameterFollowsDefaultParameter {
    parameter: &'src str,
  },
  UndefinedVariable {
    variable: &'src str,
  },
  UnexpectedCharacter {
    expected: char,
  },
  UnexpectedClosingDelimiter {
    close: Delimiter,
  },
  UnexpectedEndOfToken {
    expected: char,
  },
  UnexpectedToken {
    expected: Vec<TokenKind>,
    found: TokenKind,
  },
  UnknownAliasTarget {
    alias: &'src str,
    target: &'src str,
  },
  UnknownDependency {
    recipe: &'src str,

Review comment (suggestion):
```suggestion
  ParsingRecursionDepthExceeded,
```

Code after:
  },
  InconsistentLeadingWhitespace {
    expected: &'src str,
    found: &'src str,
  },
  Internal {
    message: String,
  },
  InvalidEscapeSequence {
    character: char,
  },
  MismatchedClosingDelimiter {
    close: Delimiter,
    open: Delimiter,
    open_line: usize,
  },
  MixedLeadingWhitespace {
    whitespace: &'src str,
  },
  ParameterFollowsVariadicParameter {
    parameter: &'src str,
  },
  ParameterShadowsVariable {
    parameter: &'src str,
  },
  ParsingRecursionDepthExceeded,
  RequiredParameterFollowsDefaultParameter {
    parameter: &'src str,
  },
  UndefinedVariable {
    variable: &'src str,
  },
  UnexpectedCharacter {
    expected: char,
  },
  UnexpectedClosingDelimiter {
    close: Delimiter,
  },
  UnexpectedEndOfToken {
    expected: char,
  },
  UnexpectedToken {
    expected: Vec<TokenKind>,
    found: TokenKind,
  },
  UnknownAliasTarget {
    alias: &'src str,
    target: &'src str,
  },
  UnknownDependency {
    recipe: &'src str,
Repository: casey/just
File: src/parser.rs (Rust)

Code before:
  /// Parse an alias, e.g `alias name := target`
  fn parse_alias(&mut self) -> CompileResult<'src, Alias<'src, Name<'src>>> {
    self.presume_keyword(Keyword::Alias)?;
    let name = self.parse_name()?;
    self.presume_any(&[Equals, ColonEquals])?;
    let target = self.parse_name()?;
    self.expect_eol()?;
    Ok(Alias { name, target })
  }

  /// Parse an assignment, e.g. `foo := bar`
  fn parse_assignment(&mut self, export: bool) -> CompileResult<'src, Assignment<'src>> {
    let name = self.parse_name()?;
    self.presume_any(&[Equals, ColonEquals])?;
    let value = self.parse_expression()?;
    self.expect_eol()?;
    Ok(Assignment {
      export,
      name,
      value,
    })
  }

  /// Parse an expression, e.g. `1 + 2`
  fn parse_expression(&mut self) -> CompileResult<'src, Expression<'src>> {
    if self.depth == 64 {
      return Err(CompileError {
        token: self.next()?,
        kind: CompileErrorKind::ParsingRecursionDepthExceeded,
      });
    }

    self.depth += 1;

    let result = if self.accepted_keyword(Keyword::If)? {
      self.parse_conditional()
    } else {
      let value = self.parse_value()?;

      if self.accepted(Plus)? {
        let lhs = Box::new(value);
        let rhs = Box::new(self.parse_expression()?);
        Ok(Expression::Concatenation { lhs, rhs })
      } else {
        Ok(value)
      }
    };

    self.depth -= 1;
    result
  }

Review comment (suggestion):
How about we just drop it down for windows:

```suggestion
    if self.depth == if cfg!(windows) { 64 } else { 256 } {
```

Code after:
  /// Parse an alias, e.g `alias name := target`
  fn parse_alias(&mut self) -> CompileResult<'src, Alias<'src, Name<'src>>> {
    self.presume_keyword(Keyword::Alias)?;
    let name = self.parse_name()?;
    self.presume_any(&[Equals, ColonEquals])?;
    let target = self.parse_name()?;
    self.expect_eol()?;
    Ok(Alias { name, target })
  }

  /// Parse an assignment, e.g. `foo := bar`
  fn parse_assignment(&mut self, export: bool) -> CompileResult<'src, Assignment<'src>> {
    let name = self.parse_name()?;
    self.presume_any(&[Equals, ColonEquals])?;
    let value = self.parse_expression()?;
    self.expect_eol()?;
    Ok(Assignment {
      export,
      name,
      value,
    })
  }

  /// Parse an expression, e.g. `1 + 2`
  fn parse_expression(&mut self) -> CompileResult<'src, Expression<'src>> {
    if self.depth == if cfg!(windows) { 64 } else { 255 } {
      return Err(CompileError {
        token: self.next()?,
        kind: CompileErrorKind::ParsingRecursionDepthExceeded,
      });
    }

    self.depth += 1;

    let expression = if self.accepted_keyword(Keyword::If)? {
      self.parse_conditional()?
    } else {
      let value = self.parse_value()?;

      if self.accepted(Plus)? {
        let lhs = Box::new(value);
        let rhs = Box::new(self.parse_expression()?);
        Expression::Concatenation { lhs, rhs }
      } else {
        value
      }
    };

    self.depth -= 1;
    Ok(expression)
  }
Repository: casey/just
File: src/parser.rs (Rust)

Code before:
    offset:  0,
    line:   0,
    column: 0,
    width:  1,
    kind: UnexpectedToken {
      expected: vec![At, Comment, Eof, Eol, Identifier],
      found: BraceL,
    },
  }

  error! {
    name:   unclosed_parenthesis_in_expression,
    input:  "x := foo(",
    offset: 9,
    line:   0,
    column: 9,
    width:  0,
    kind: UnexpectedToken{
      expected: vec![
        Backtick,
        Identifier,
        ParenL,
        ParenR,
        Slash,
        StringToken,
      ],
      found: Eof,
    },
  }

  error! {
    name:   unclosed_parenthesis_in_interpolation,
    input:  "a:\n echo {{foo(}}",
    offset:  15,
    line:   1,
    column: 12,
    width:  2,
    kind:   UnexpectedToken{
      expected: vec![
        Backtick,
        Identifier,
        ParenL,
        ParenR,
        Slash,
        StringToken,
      ],
      found: InterpolationEnd,
    },
  }

  error! {

Review comment (bug):
I wasn't able to confidently understand what these tests mean. My best guess was that these are the tokens that can be found at the beginning of an expression without causing an exception.

This was the output from one of the two failed tests:
```
---- parser::tests::unclosed_parenthesis_in_interpolation stdout ----
thread 'parser::tests::unclosed_parenthesis_in_interpolation' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
 CompileError {
     token: Token {
         offset: 15,
         length: 2,
         line: 1,
         column: 12,
         src: "a:\n echo {{foo(}}",
         kind: InterpolationEnd,
     },
     kind: UnexpectedToken {
         expected: [
             Backtick,
             Identifier,
             ParenL,
             ParenR,
<            Slash,
             StringToken,
         ],
         found: InterpolationEnd,
     },
 }

', src/parser.rs:907:9
```

Code after:
    name:   unexpected_brace,
    input:  "{{",
    offset:  0,
    line:   0,
    column: 0,
    width:  1,
    kind: UnexpectedToken {
      expected: vec![At, Comment, Eof, Eol, Identifier],
      found: BraceL,
    },
  }

  error! {
    name:   unclosed_parenthesis_in_expression,
    input:  "x := foo(",
    offset: 9,
    line:   0,
    column: 9,
    width:  0,
    kind: UnexpectedToken{
      expected: vec![
        Backtick,
        Identifier,
        ParenL,
        ParenR,
        Slash,
        StringToken,
      ],
      found: Eof,
    },
  }

  error! {
    name:   unclosed_parenthesis_in_interpolation,
    input:  "a:\n echo {{foo(}}",
    offset:  15,
    line:   1,
    column: 12,
    width:  2,
    kind:   UnexpectedToken{
      expected: vec![
        Backtick,
        Identifier,
        ParenL,
        ParenR,
        Slash,
        StringToken,
      ],
      found: InterpolationEnd,
    },
  }
Repository: casey/just
File: src/node.rs (Rust)

Code before:
            args: [a, b, c],
            ..
          } => {
            tree.push_mut(name.lexeme());
            tree.push_mut(a.tree());
            tree.push_mut(b.tree());
            tree.push_mut(c.tree());
          }
        }

        tree
      }
      Expression::Variable { name } => Tree::atom(name.lexeme()),
      Expression::StringLiteral {
        string_literal: StringLiteral { cooked, .. },
      } => Tree::string(cooked),
      Expression::Backtick { contents, .. } => Tree::atom("backtick").push(Tree::string(contents)),
      Expression::Group { contents } => Tree::List(vec![contents.tree()]),
      Expression::Join { lhs, rhs } => Tree::atom("/")
        .push(
          lhs
            .as_ref()
            .unwrap_or(&Box::new(Expression::empty_string_literal()))
            .tree(),
        )
        .push(rhs.tree()),
    }
  }
}

impl<'src> Node<'src> for UnresolvedRecipe<'src> {
  fn tree(&self) -> Tree<'src> {
    let mut t = Tree::atom("recipe");

    if self.quiet {
      t.push_mut("#");
      t.push_mut("quiet");
    }

    if let Some(doc) = self.doc {
      t.push_mut(Tree::string(doc));
    }

    t.push_mut(self.name.lexeme());

    if !self.parameters.is_empty() {
      let mut params = Tree::atom("params");

      for parameter in &self.parameters {
        if let Some(prefix) = parameter.kind.prefix() {
          params.push_mut(prefix);

Review comment (suggestion):
```suggestion
Expression::Join { None, rhs } => Tree::atom("/")
  .push(rhs.tree()),
Expression::Join { Some(lhs), rhs } => Tree::atom("/")
  .push(lhs.tree()),
  .push(rhs.tree()),
```

Code after:
            args: [a, b, c],
            ..
          } => {
            tree.push_mut(name.lexeme());
            tree.push_mut(a.tree());
            tree.push_mut(b.tree());
            tree.push_mut(c.tree());
          }
        }

        tree
      }
      Expression::Variable { name } => Tree::atom(name.lexeme()),
      Expression::StringLiteral {
        string_literal: StringLiteral { cooked, .. },
      } => Tree::string(cooked),
      Expression::Backtick { contents, .. } => Tree::atom("backtick").push(Tree::string(contents)),
      Expression::Group { contents } => Tree::List(vec![contents.tree()]),
      Expression::Join { lhs: None, rhs } => Tree::atom("/").push(rhs.tree()),
      Expression::Join {
        lhs: Some(lhs),
        rhs,
      } => Tree::atom("/").push(lhs.tree()).push(rhs.tree()),
    }
  }
}

impl<'src> Node<'src> for UnresolvedRecipe<'src> {
  fn tree(&self) -> Tree<'src> {
    let mut t = Tree::atom("recipe");

    if self.quiet {
      t.push_mut("#");
      t.push_mut("quiet");
    }

    if let Some(doc) = self.doc {
      t.push_mut(Tree::string(doc));
    }

    t.push_mut(self.name.lexeme());

    if !self.parameters.is_empty() {
      let mut params = Tree::atom("params");

      for parameter in &self.parameters {
        if let Some(prefix) = parameter.kind.prefix() {
          params.push_mut(prefix);
        }

        params.push_mut(parameter.tree());
Repository: casey/just
File: src/run.rs (Rust)

Code before:
use super::*;

/// Main entry point into just.
pub fn run() -> Result<(), i32> {
  #[cfg(windows)]
  ansi_term::enable_ansi_support().ok();

  env_logger::Builder::from_env(
    env_logger::Env::new()
      .filter("JUST_LOG")
      .write_style("JUST_LOG_STYLE"),
  )
  .init();

  let app = Config::app();

  info!("Parsing command line arguments…");
  let matches = app.get_matches();

  let loader = Loader::new();

  let config = Config::from_matches(&matches).map_err(Error::from);

  let (color, verbosity) = config
    .as_ref()
    .map(|config| (config.color, config.verbosity))
    .unwrap_or((Color::auto(), Verbosity::default()));


Review comment (suggestion):
```suggestion
/// Main entry point into just binary.
```

Code after:
use super::*;

/// Main entry point into just binary.
pub fn run() -> Result<(), i32> {
  #[cfg(windows)]
  ansi_term::enable_ansi_support().ok();

  env_logger::Builder::from_env(
    env_logger::Env::new()
      .filter("JUST_LOG")
      .write_style("JUST_LOG_STYLE"),
  )
  .init();

  let app = Config::app();

  info!("Parsing command line arguments…");
  let matches = app.get_matches();

  let loader = Loader::new();

  let config = Config::from_matches(&matches).map_err(Error::from);

  let (color, verbosity) = config
    .as_ref()
    .map(|config| (config.color, config.verbosity))
    .unwrap_or((Color::auto(), Verbosity::default()));
Repository: casey/just
File: src/attribute.rs (Rust)

Code before:
use super::*;

#[derive(EnumString)]
#[strum(serialize_all = "kebab_case")]
pub(crate) enum Attribute {
  NoExitMessage,
}

impl Attribute {
  pub(crate) fn from_lexeme(lexeme: &str) -> Option<Attribute> {
    lexeme.parse().ok()
  }
}


Review comment (suggestion):
I think this is a little clearer, since attributes should only be parsed from naems:

```suggestion
  pub(crate) fn from_name(lexeme: Name) -> Option<Attribute> {
    name.lexeme().parse().ok()
  }
```

Code after:
use super::*;

#[derive(EnumString)]
#[strum(serialize_all = "kebab_case")]
pub(crate) enum Attribute {
  NoExitMessage,
}

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