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

Code before:
        module
          .name
          .map(|name| name.token.offset)
          .unwrap_or_default()
      });
    }

    modules
  }

  pub(crate) fn public_recipes(&self, config: &Config) -> Vec<&Recipe> {
    let mut recipes = self
      .recipes
      .values()
      .map(AsRef::as_ref)
      .filter(|recipe| recipe.is_public())
      .collect::<Vec<&Recipe>>();

    if config.unsorted {
      recipes.sort_by_key(|recipe| (&recipe.import_offsets, recipe.name.offset));
    }

    recipes
  }

  pub(crate) fn groups(&self) -> Vec<String> {
    self.groups.clone()
  }

  pub(crate) fn public_groups(&self, config: &Config) -> Vec<String> {
    let mut groups = Vec::new();

    for recipe in self.recipes.values() {
      if recipe.is_public() {
        for group in recipe.groups() {
          groups.push((&recipe.import_offsets, recipe.name.offset, group));
        }
      }
    }

    let empty_vec: Vec<usize> = Vec::new();
    for submodule in self.modules.values() {
      for group in submodule.groups() {
        groups.push((&empty_vec, 0, group));
      }
    }

    if config.unsorted {
      groups.sort();
    } else {
      groups.sort_by(|(_, _, a), (_, _, b)| a.cmp(b));

Review comment (suggestion):
This can return `&[String]`, so that we can avoid a clone of the whole vec.

Code after:
        module
          .name
          .map(|name| name.token.offset)
          .unwrap_or_default()
      });
    }

    modules
  }

  pub(crate) fn public_recipes(&self, config: &Config) -> Vec<&Recipe> {
    let mut recipes = self
      .recipes
      .values()
      .map(AsRef::as_ref)
      .filter(|recipe| recipe.is_public())
      .collect::<Vec<&Recipe>>();

    if config.unsorted {
      recipes.sort_by_key(|recipe| (&recipe.import_offsets, recipe.name.offset));
    }

    recipes
  }

  pub(crate) fn groups(&self) -> &[String] {
    &self.groups
  }

  pub(crate) fn public_groups(&self, config: &Config) -> Vec<String> {
    let mut groups = Vec::new();

    for recipe in self.recipes.values() {
      if recipe.is_public() {
        for group in recipe.groups() {
          groups.push((recipe.import_offsets.as_slice(), recipe.name.offset, group));
        }
      }
    }

    for submodule in self.modules.values() {
      for group in submodule.groups() {
        groups.push((&[], submodule.name.unwrap().offset, group.to_string()));
      }
    }

    if config.unsorted {
      groups.sort();
    } else {
      groups.sort_by(|(_, _, a), (_, _, b)| a.cmp(b));
    }
Repository: casey/just
File: src/justfile.rs (Rust)

Code before:
    if config.unsorted {
      recipes.sort_by_key(|recipe| (&recipe.import_offsets, recipe.name.offset));
    }

    recipes
  }

  pub(crate) fn groups(&self) -> Vec<String> {
    self.groups.clone()
  }

  pub(crate) fn public_groups(&self, config: &Config) -> Vec<String> {
    let mut groups = Vec::new();

    for recipe in self.recipes.values() {
      if recipe.is_public() {
        for group in recipe.groups() {
          groups.push((&recipe.import_offsets, recipe.name.offset, group));
        }
      }
    }

    let empty_vec: Vec<usize> = Vec::new();
    for submodule in self.modules.values() {
      for group in submodule.groups() {
        groups.push((&empty_vec, 0, group));
      }
    }

    if config.unsorted {
      groups.sort();
    } else {
      groups.sort_by(|(_, _, a), (_, _, b)| a.cmp(b));
    }

    let mut seen = HashSet::new();

    groups.retain(|(_, _, group)| seen.insert(group.clone()));

    groups.into_iter().map(|(_, _, group)| group).collect()
  }
}

impl<'src> ColorDisplay for Justfile<'src> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    let mut items = self.recipes.len() + self.assignments.len() + self.aliases.len();
    for (name, assignment) in &self.assignments {
      if assignment.export {
        write!(f, "export ")?;
      }
      write!(f, "{name} := {}", assignment.value)?;

Review comment (suggestion):
This can be:
```suggestion
        groups.push((&[], 0, group));
```

Code after:
    if config.unsorted {
      recipes.sort_by_key(|recipe| (&recipe.import_offsets, recipe.name.offset));
    }

    recipes
  }

  pub(crate) fn groups(&self) -> &[String] {
    &self.groups
  }

  pub(crate) fn public_groups(&self, config: &Config) -> Vec<String> {
    let mut groups = Vec::new();

    for recipe in self.recipes.values() {
      if recipe.is_public() {
        for group in recipe.groups() {
          groups.push((recipe.import_offsets.as_slice(), recipe.name.offset, group));
        }
      }
    }

    for submodule in self.modules.values() {
      for group in submodule.groups() {
        groups.push((&[], submodule.name.unwrap().offset, group.to_string()));
      }
    }

    if config.unsorted {
      groups.sort();
    } else {
      groups.sort_by(|(_, _, a), (_, _, b)| a.cmp(b));
    }

    let mut seen = HashSet::new();

    groups.retain(|(_, _, group)| seen.insert(group.clone()));

    groups.into_iter().map(|(_, _, group)| group).collect()
  }
}

impl<'src> ColorDisplay for Justfile<'src> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    let mut items = self.recipes.len() + self.assignments.len() + self.aliases.len();
    for (name, assignment) in &self.assignments {
      if assignment.export {
        write!(f, "export ")?;
      }
      write!(f, "{name} := {}", assignment.value)?;
      items -= 1;
Repository: casey/just
File: tests/modules.rs (Rust)

Code before:
    )
    .test_round_trip(false)
    .arg("--list")
    .stdout("Available recipes:\n    foo ... # Comment\n")
    .run();
}

#[test]
fn doc_attribute_on_module() {
  Test::new()
    .write("foo.just", "")
    .justfile(
      r#"
        # Suppressed comment
        [doc: "Comment"]
        mod foo
      "#,
    )
    .test_round_trip(false)
    .arg("--list")
    .stdout("Available recipes:\n    foo ... # Comment\n")
    .run();
}

#[test]
fn group_attribute_on_module() {
  Test::new()
    .write("foo.just", "")
    .write("bar.just", "")
    .justfile(
      r#"
        [group('alpha')]
        mod foo

        [group('alpha')]
        a:
  
        [group('beta')]
        b:
        [group('beta')]
        mod bar
        c:
      "#,
    )
    .test_round_trip(false)
    .arg("--list")
    .stdout(
      "
        Available recipes:
            (no group)
            c

Review comment (suggestion):
We should have a test for the ordering of submodules in groups if `--unsorted` is used.

My first thought was that submodules should be ordered based on their position in the `justfile`. However, I'm now not sure if that's correct. I think it's natural to have submodules all declared together at the top or bottom of a `justfile`, and natural to list submodules at the end of each group.

So maybe my comment above is off, and we should, instead of using `submodule.name.unwrap().offset` in the ordering key, use `usize::MAX` instead.

Code after:
    )
    .test_round_trip(false)
    .arg("--list")
    .stdout("Available recipes:\n    foo ... # Comment\n")
    .run();
}

#[test]
fn doc_attribute_on_module() {
  Test::new()
    .write("foo.just", "")
    .justfile(
      r#"
        # Suppressed comment
        [doc: "Comment"]
        mod foo
      "#,
    )
    .test_round_trip(false)
    .arg("--list")
    .stdout("Available recipes:\n    foo ... # Comment\n")
    .run();
}

#[test]
fn group_attribute_on_module() {
  Test::new()
    .write("foo.just", "")
    .write("bar.just", "")
    .write("zee.just", "")
    .justfile(
      r#"
        [group('alpha')]
        mod zee

        [group('alpha')]
        mod foo

        [group('alpha')]
        a:

        [group('beta')]
        b:

        [group('beta')]
        mod bar

        c:
      "#,
    )
    .test_round_trip(false)
Repository: casey/just
File: tests/misc.rs (Rust)

Code before:
use super::*;

test! {
  name: alias_listing,
  justfile: "
    foo:
      echo foo

    alias f := foo
  ",
  args: ("--list"),
  stdout: "
    Available recipes:
        foo # [aliases: f]
  ",
}

test! {
  name: alias_listing_with_doc,
  justfile: "
    # foo command
    foo:
      echo foo

    alias f := foo
  ",
  args: ("--list"),
  stdout: "
    Available recipes:
        foo # foo command [aliases: f]
  ",
}

test! {
  name: alias_listing_multiple_aliases,
  justfile: "foo:\n  echo foo\nalias f := foo\nalias fo := foo",
  args: ("--list"),
  stdout: "
    Available recipes:
        foo # [aliases: f, fo]
  ",
}


Review comment (suggestion):
I'm actually trying to avoid creating new tests which use the `test!` macro. This should be converted to use the `Test` struct directly. I found that the `test!` macro style was a bit inflexible.

Code after:
use super::*;

test! {
  name: alias_listing,
  justfile: "
    foo:
      echo foo

    alias f := foo
  ",
  args: ("--list"),
  stdout: "
    Available recipes:
        foo # [alias: f]
  ",
}

#[test]
fn alias_listing_with_doc() {
  Test::new()
    .justfile(
      "
        # foo command
        foo:
          echo foo

        alias f := foo
      ",
    )
    .arg("--list")
    .stdout(
      "
      Available recipes:
          foo # foo command [alias: f]
    ",
    )
    .run();
}

test! {
  name: alias_listing_multiple_aliases,
  justfile: "foo:\n  echo foo\nalias f := foo\nalias fo := foo",
  args: ("--list"),
Repository: casey/just
File: src/subcommand.rs (Rust)

Code before:
      }

      if let Some(recipes) = recipe_groups.get(&group) {
        for recipe in recipes {
          for (i, name) in iter::once(&recipe.name())
            .chain(aliases.get(recipe.name()).unwrap_or(&Vec::new()))
            .enumerate()
          {
            let doc = if i == 0 {
              recipe.doc().map(Cow::Borrowed)
            } else {
              Some(Cow::Owned(format!("alias for `{}`", recipe.name)))
            };

            if let Some(doc) = &doc {
              if doc.lines().count() > 1 {
                for line in doc.lines() {
                  println!(
                    "{list_prefix}{} {}",
                    config.color.stdout().doc().paint("#"),
                    config.color.stdout().doc().paint(line),
                  );
                }
              }
            }

            if i == 0 || config.no_inline_aliases {
              print!(
                "{list_prefix}{}",
                RecipeSignature { name, recipe }.color_display(config.color.stdout())
              );

              format_doc(
                config,
                name,
                doc.as_deref(),
                aliases
                  .get(recipe.name())
                  .filter(|_| !config.no_inline_aliases)
                  .cloned(),
                max_signature_width,
                &signature_widths,
              );
            }
          }
        }
      }

      if let Some(submodules) = submodule_groups.get(&group) {
        for (i, submodule) in submodules.iter().enumerate() {
          if config.list_submodules {

Review comment (suggestion):
I think if `no_inline_aliases` is false, i.e., we are printing aliases inline, we should modify the iterator above to not inline the aliases.

Code after:
      }
      groups
    };

    let submodule_groups = {
      let mut groups = BTreeMap::<Option<String>, Vec<&Justfile>>::new();
      for submodule in module.modules(config) {
        let submodule_groups = submodule.groups();
        if submodule_groups.is_empty() {
          groups.entry(None).or_default().push(submodule);
        } else {
          for group in submodule_groups {
            groups
              .entry(Some(group.to_string()))
              .or_default()
              .push(submodule);
          }
        }
      }
      groups
    };

    let mut ordered_groups = module
      .public_groups(config)
      .into_iter()
      .map(Some)
      .collect::<Vec<Option<String>>>();

    if recipe_groups.contains_key(&None) || submodule_groups.contains_key(&None) {
      ordered_groups.insert(0, None);
    }

    let no_groups = ordered_groups.len() == 1 && ordered_groups.first() == Some(&None);
    let mut groups_count = 0;
    if !no_groups {
      groups_count = ordered_groups.len();
    }

    for (i, group) in ordered_groups.into_iter().enumerate() {
      if i > 0 {
        println!();
      }

      if !no_groups {
        if let Some(group) = &group {
          println!(
            "{list_prefix}{}",
            config.color.stdout().group().paint(&format!("[{group}]"))
          );
        }
      }
Repository: casey/just
File: src/attribute.rs (Rust)

Code before:
      | Self::Group(argument) => write!(f, "({argument})")?,
      Self::Script(Some(shell)) => write!(f, "({shell})")?,
      Self::Confirm(None)
      | Self::Doc(None)
      | Self::Linux
      | Self::Macos
      | Self::NoCd
      | Self::NoExitMessage
      | Self::NoQuiet
      | Self::PositionalArguments
      | Self::Private
      | Self::Script(None)
      | Self::Unix
      | Self::Windows => {}
    }

    Ok(())
  }
}

#[derive(Default, Debug, Clone, PartialEq)]
pub(crate) struct AttributeSet<'src> {
  inner: BTreeSet<Attribute<'src>>,
}

impl<'src> Serialize for AttributeSet<'src> {
  fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
  where
    S: Serializer,
  {
    self.inner.serialize(serializer)
  }
}

impl<'src> AttributeSet<'src> {
  pub(crate) fn from_iter(iter: impl IntoIterator<Item = Attribute<'src>>) -> Self {
    Self {
      inner: iter.into_iter().collect(),
    }
  }

  pub(crate) fn count(&self) -> usize {
    self.inner.len()
  }

  pub(crate) fn contains(&self, target: AttributeDiscriminant) -> bool {
    self.inner.iter().any(|attr| {
      let discriminant: AttributeDiscriminant = attr.into();
      discriminant == target
    })
  }

Review comment (suggestion):
Can we avoid needing to write a custom a serialize implementation if we make the struct:

```
pub(crate) struct AttributeSet<'src>(BTreeSet<Attribute<'src>>);
```

I think in that case serde will serialize the struct as just the inner contents.


Code after:
  }

  pub(crate) fn repeatable(&self) -> bool {
    matches!(self, Attribute::Group(_))
  }
}

impl Display for Attribute<'_> {
  fn fmt(&self, f: &mut Formatter) -> fmt::Result {
    write!(f, "{}", self.name())?;

    match self {
      Self::Confirm(Some(argument))
      | Self::Doc(Some(argument))
      | Self::Extension(argument)
      | Self::Group(argument)
      | Self::WorkingDirectory(argument) => write!(f, "({argument})")?,
      Self::Script(Some(shell)) => write!(f, "({shell})")?,
      Self::Confirm(None)
      | Self::Doc(None)
      | Self::Linux
      | Self::Macos
      | Self::NoCd
      | Self::NoExitMessage
      | Self::NoQuiet
      | Self::Openbsd
      | Self::PositionalArguments
      | Self::Private
      | Self::Script(None)
      | Self::Unix
      | Self::Windows => {}
    }

    Ok(())
  }
}

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

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

Code before:
    Ok(())
  }
}

#[derive(Default, Debug, Clone, PartialEq)]
pub(crate) struct AttributeSet<'src> {
  inner: BTreeSet<Attribute<'src>>,
}

impl<'src> Serialize for AttributeSet<'src> {
  fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
  where
    S: Serializer,
  {
    self.inner.serialize(serializer)
  }
}

impl<'src> AttributeSet<'src> {
  pub(crate) fn from_iter(iter: impl IntoIterator<Item = Attribute<'src>>) -> Self {
    Self {
      inner: iter.into_iter().collect(),
    }
  }

  pub(crate) fn count(&self) -> usize {
    self.inner.len()
  }

  pub(crate) fn contains(&self, target: AttributeDiscriminant) -> bool {
    self.inner.iter().any(|attr| {
      let discriminant: AttributeDiscriminant = attr.into();
      discriminant == target
    })
  }

  pub(crate) fn get(&self, discriminant: AttributeDiscriminant) -> Option<&Attribute<'src>> {
    self.inner.iter().find(|attr| {
      let item_discriminant: AttributeDiscriminant = (*attr).into();
      discriminant == item_discriminant
    })
  }

  pub(crate) fn iter(&self) -> impl Iterator<Item = &Attribute<'src>> {
    self.inner.iter()
  }

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


Review comment (suggestion):
I think we should just call this `len`, since that's what native collections use.

Code after:
      | Self::WorkingDirectory(argument) => write!(f, "({argument})")?,
      Self::Script(Some(shell)) => write!(f, "({shell})")?,
      Self::Confirm(None)
      | Self::Doc(None)
      | Self::Linux
      | Self::Macos
      | Self::NoCd
      | Self::NoExitMessage
      | Self::NoQuiet
      | Self::Openbsd
      | Self::PositionalArguments
      | Self::Private
      | Self::Script(None)
      | Self::Unix
      | Self::Windows => {}
    }

    Ok(())
  }
}

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

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

Code before:
              code,
              print_message: self.print_exit_message(),
            })
          }
        },
      ),
      Err(io_error) => Err(executor.error(io_error, self.name())),
    }
  }

  pub(crate) fn groups(&self) -> BTreeSet<String> {
    self
      .attributes
      .iter()
      .filter_map(|attribute| {
        if let Attribute::Group(group) = attribute {
          Some(group.cooked.clone())
        } else {
          None
        }
      })
      .collect()
  }

  pub(crate) fn doc(&self) -> Option<&str> {
    if let Some(Attribute::Doc(doc)) = self.attributes.get(AttributeDiscriminant::Doc) {
      return doc.as_ref().map(|s| s.cooked.as_ref());
    }
    self.doc
  }

  pub(crate) fn subsequents(&self) -> impl Iterator<Item = &D> {
    self.dependencies.iter().skip(self.priors)
  }
}

impl<'src, D: Display> ColorDisplay for Recipe<'src, D> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    if let Some(doc) = self.doc {
      writeln!(f, "# {doc}")?;
    }

    for attribute in self.attributes.iter() {
      writeln!(f, "[{attribute}]")?;
    }

    if self.quiet {
      write!(f, "@{}", self.name)?;
    } else {
      write!(f, "{}", self.name)?;
    }

Review comment (suggestion):
This is kind of a weird one. This `if let` will only match if *both* `self.attributes.get` returns `Some`, and the returned attribute is an `Attribute::Doc`, if the code is correct, of course, both or neither will be the case.

I think a more scrupulous correct version would be:

```
if let Some(attribute) = self.attributes.get(AttributeDiscriminant::Doc) {
  if let Some(Attribute::Doc(doc)) else {
    unreachable!();
  }
}
```

Although that's a little crazy, so I think maybe the original on the left is better.

Code after:
        |code| {
          if code == 0 {
            Ok(())
          } else {
            Err(Error::Code {
              recipe: self.name(),
              line_number: None,
              code,
              print_message: self.print_exit_message(),
            })
          }
        },
      ),
      Err(io_error) => Err(executor.error(io_error, self.name())),
    }
  }

  pub(crate) fn groups(&self) -> BTreeSet<String> {
    self
      .attributes
      .iter()
      .filter_map(|attribute| {
        if let Attribute::Group(group) = attribute {
          Some(group.cooked.clone())
        } else {
          None
        }
      })
      .collect()
  }

  pub(crate) fn doc(&self) -> Option<&str> {
    for attribute in &self.attributes {
      if let Attribute::Doc(doc) = attribute {
        return doc.as_ref().map(|s| s.cooked.as_ref());
      }
    }

    self.doc.as_deref()
  }

  pub(crate) fn subsequents(&self) -> impl Iterator<Item = &D> {
    self.dependencies.iter().skip(self.priors)
  }
}

impl<D: Display> ColorDisplay for Recipe<'_, D> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    if !self
      .attributes
      .iter()
Repository: casey/just
File: src/recipe.rs (Rust)

Code before:
        } else {
          None
        }
      })
      .collect()
  }

  pub(crate) fn doc(&self) -> Option<&str> {
    if let Some(Attribute::Doc(doc)) = self.attributes.get(AttributeDiscriminant::Doc) {
      return doc.as_ref().map(|s| s.cooked.as_ref());
    }
    self.doc
  }

  pub(crate) fn subsequents(&self) -> impl Iterator<Item = &D> {
    self.dependencies.iter().skip(self.priors)
  }
}

impl<'src, D: Display> ColorDisplay for Recipe<'src, D> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    if let Some(doc) = self.doc {
      writeln!(f, "# {doc}")?;
    }

    for attribute in self.attributes.iter() {
      writeln!(f, "[{attribute}]")?;
    }

    if self.quiet {
      write!(f, "@{}", self.name)?;
    } else {
      write!(f, "{}", self.name)?;
    }

    for parameter in &self.parameters {
      write!(f, " {}", parameter.color_display(color))?;
    }
    write!(f, ":")?;

    for (i, dependency) in self.dependencies.iter().enumerate() {
      if i == self.priors {
        write!(f, " &&")?;
      }

      write!(f, " {dependency}")?;
    }

    for (i, line) in self.body.iter().enumerate() {
      if i == 0 {
        writeln!(f)?;

Review comment (suggestion):
We should implement `IntoIterator` for `&AttributeSet` so we can avoid needing to call `.iter()`.

Code after:
  pub(crate) fn groups(&self) -> BTreeSet<String> {
    self
      .attributes
      .iter()
      .filter_map(|attribute| {
        if let Attribute::Group(group) = attribute {
          Some(group.cooked.clone())
        } else {
          None
        }
      })
      .collect()
  }

  pub(crate) fn doc(&self) -> Option<&str> {
    for attribute in &self.attributes {
      if let Attribute::Doc(doc) = attribute {
        return doc.as_ref().map(|s| s.cooked.as_ref());
      }
    }

    self.doc.as_deref()
  }

  pub(crate) fn subsequents(&self) -> impl Iterator<Item = &D> {
    self.dependencies.iter().skip(self.priors)
  }
}

impl<D: Display> ColorDisplay for Recipe<'_, D> {
  fn fmt(&self, f: &mut Formatter, color: Color) -> fmt::Result {
    if !self
      .attributes
      .iter()
      .any(|attribute| matches!(attribute, Attribute::Doc(_)))
    {
      if let Some(doc) = &self.doc {
        writeln!(f, "# {doc}")?;
      }
    }

    for attribute in &self.attributes {
      writeln!(f, "[{attribute}]")?;
    }

    if self.quiet {
      write!(f, "@{}", self.name)?;
    } else {
      write!(f, "{}", self.name)?;
    }