Repository: casey/just
Pull request #2263 (2024-07-19 04:22:42)

feat: allow groups on submodules
Hello! I've been working on a fix for https://github.com/casey/just/issues/2243.

I added the whole `attributes` object instead of just passing around the group like how the `doc` is. My rationale for this was to make it easier to have access to anything stored in the `attributes` in the future, let me know if that is the wrong path though.

This is my first time contributing to `just`, I'm open to any feedback!

jmwoliver: @casey Thanks for the feedback! Great points, I'll get this updated to just pass in the group names instead.
jmwoliver: @casey I switched to passing around `groups: Vec<String>`, hopefully this looks better!
jmwoliver: Fixed linter errors and re-pushed.
jmwoliver: @casey Thanks for the all the great feedback and the `just ci` tip. I've updated everything with your recommendation, let me know how it looks!

Patch:
diff --git a/src/analyzer.rs b/src/analyzer.rs
index cdb598d8..661e7092 100644
--- a/src/analyzer.rs
+++ b/src/analyzer.rs
@@ -11,18 +11,20 @@ impl<'src> Analyzer<'src> {
   pub(crate) fn analyze(
     asts: &HashMap<PathBuf, Ast<'src>>,
     doc: Option<String>,
+    groups: &[String],
     loaded: &[PathBuf],
     name: Option<Name<'src>>,
     paths: &HashMap<PathBuf, PathBuf>,
     root: &Path,
   ) -> CompileResult<'src, Justfile<'src>> {
-    Self::default().justfile(asts, doc, loaded, name, paths, root)
+    Self::default().justfile(asts, doc, groups, loaded, name, paths, root)
   }
 
   fn justfile(
     mut self,
     asts: &HashMap<PathBuf, Ast<'src>>,
     doc: Option<String>,
+    groups: &[String],
     loaded: &[PathBuf],
     name: Option<Name<'src>>,
     paths: &HashMap<PathBuf, PathBuf>,
@@ -94,9 +96,12 @@ impl<'src> Analyzer<'src> {
             ..
           } => {
             let mut doc_attr: Option<&str> = None;
+            let mut groups = Vec::new();
             for attribute in attributes {
               if let Attribute::Doc(ref doc) = attribute {
                 doc_attr = Some(doc.as_ref().map(|s| s.cooked.as_ref()).unwrap_or_default());
+              } else if let Attribute::Group(ref group) = attribute {
+                groups.push(group.cooked.clone());
               } else {
                 return Err(name.token.error(InvalidAttribute {
                   item_kind: "Module",
@@ -111,6 +116,7 @@ impl<'src> Analyzer<'src> {
               modules.insert(Self::analyze(
                 asts,
                 doc_attr.or(*doc).map(ToOwned::to_owned),
+                groups.as_slice(),
                 loaded,
                 Some(*name),
                 paths,
@@ -216,6 +222,7 @@ impl<'src> Analyzer<'src> {
           }),
         }),
       doc,
+      groups: groups.into(),
       loaded: loaded.into(),
       modules,
       name,
diff --git a/src/compiler.rs b/src/compiler.rs
index 119a0af5..322b1d20 100644
--- a/src/compiler.rs
+++ b/src/compiler.rs
@@ -96,7 +96,7 @@ impl Compiler {
       asts.insert(current.path, ast.clone());
     }
 
-    let justfile = Analyzer::analyze(&asts, None, &loaded, None, &paths, root)?;
+    let justfile = Analyzer::analyze(&asts, None, &[], &loaded, None, &paths, root)?;
 
     Ok(Compilation {
       asts,
@@ -229,7 +229,7 @@ impl Compiler {
     asts.insert(root.clone(), ast);
     let mut paths: HashMap<PathBuf, PathBuf> = HashMap::new();
     paths.insert(root.clone(), root.clone());
-    Analyzer::analyze(&asts, None, &[], None, &paths, &root)
+    Analyzer::analyze(&asts, None, &[], &[], None, &paths, &root)
   }
 }
 
diff --git a/src/justfile.rs b/src/justfile.rs
index 90bfc5e7..ab8ee547 100644
--- a/src/justfile.rs
+++ b/src/justfile.rs
@@ -18,6 +18,7 @@ pub(crate) struct Justfile<'src> {
   pub(crate) default: Option<Rc<Recipe<'src>>>,
   #[serde(skip)]
   pub(crate) loaded: Vec<PathBuf>,
+  pub(crate) groups: Vec<String>,
   pub(crate) modules: Table<'src, Justfile<'src>>,
   #[serde(skip)]
   pub(crate) name: Option<Name<'src>>,
@@ -395,17 +396,27 @@ impl<'src> Justfile<'src> {
     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, recipe.name.offset, group));
+          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 {
diff --git a/src/subcommand.rs b/src/subcommand.rs
index 3444a251..0ab08a87 100644
--- a/src/subcommand.rs
+++ b/src/subcommand.rs
@@ -520,7 +520,7 @@ impl Subcommand {
       print!("{}", config.list_heading);
     }
 
-    let groups = {
+    let recipe_groups = {
       let mut groups = BTreeMap::<Option<String>, Vec<&Recipe>>::new();
       for recipe in module.public_recipes(config) {
         let recipe_groups = recipe.groups();
@@ -535,19 +535,41 @@ impl Subcommand {
       groups
     };
 
-    let mut ordered = module
+    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 groups.contains_key(&None) {
-      ordered.insert(0, None);
+    if recipe_groups.contains_key(&None) || submodule_groups.contains_key(&None) {
+      ordered_groups.insert(0, None);
     }
 
-    let no_groups = groups.contains_key(&None) && groups.len() == 1;
+    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.into_iter().enumerate() {
+    for (i, group) in ordered_groups.into_iter().enumerate() {
       if i > 0 {
         println!();
       }
@@ -561,69 +583,66 @@ impl Subcommand {
         }
       }
 
-      for recipe in groups.get(&group).unwrap() {
-        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 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),
+                  );
+                }
               }
             }
-          }
-
-          print!(
-            "{list_prefix}{}",
-            RecipeSignature { name, recipe }.color_display(config.color.stdout())
-          );
 
-          format_doc(
-            config,
-            name,
-            doc.as_deref(),
-            max_signature_width,
-            &signature_widths,
-          );
+            print!(
+              "{list_prefix}{}",
+              RecipeSignature { name, recipe }.color_display(config.color.stdout())
+            );
+
+            format_doc(
+              config,
+              name,
+              doc.as_deref(),
+              max_signature_width,
+              &signature_widths,
+            );
+          }
         }
       }
-    }
-
-    if config.list_submodules {
-      for (i, submodule) in module.modules(config).into_iter().enumerate() {
-        if i + groups.len() > 0 {
-          println!();
-        }
 
-        println!("{list_prefix}{}:", submodule.name());
+      if let Some(submodules) = submodule_groups.get(&group) {
+        for (i, submodule) in submodules.iter().enumerate() {
+          if config.list_submodules {
+            if no_groups && (i + groups_count > 0) {
+              println!();
+            }
+            println!("{list_prefix}{}:", submodule.name());
 
-        Self::list_module(config, submodule, depth + 1);
-      }
-    } else {
-      for (i, submodule) in module.modules(config).into_iter().enumerate() {
-        if !no_groups && !groups.is_empty() && i == 0 {
-          println!();
+            Self::list_module(config, submodule, depth + 1);
+          } else {
+            print!("{list_prefix}{} ...", submodule.name());
+            format_doc(
+              config,
+              submodule.name(),
+              submodule.doc.as_deref(),
+              max_signature_width,
+              &signature_widths,
+            );
+          }
         }
-
-        print!("{list_prefix}{} ...", submodule.name());
-        format_doc(
-          config,
-          submodule.name(),
-          submodule.doc.as_deref(),
-          max_signature_width,
-          &signature_widths,
-        );
       }
     }
   }


Test patch:
diff --git a/src/testing.rs b/src/testing.rs
index 06bcde27..a43262f1 100644
--- a/src/testing.rs
+++ b/src/testing.rs
@@ -77,7 +77,7 @@ pub(crate) fn analysis_error(
   let mut paths: HashMap<PathBuf, PathBuf> = HashMap::new();
   paths.insert("justfile".into(), "justfile".into());
 
-  match Analyzer::analyze(&asts, None, &[], None, &paths, &root) {
+  match Analyzer::analyze(&asts, None, &[], &[], None, &paths, &root) {
     Ok(_) => panic!("Analysis unexpectedly succeeded"),
     Err(have) => {
       let want = CompileError {
diff --git a/tests/json.rs b/tests/json.rs
index 70b19304..19135aca 100644
--- a/tests/json.rs
+++ b/tests/json.rs
@@ -27,6 +27,7 @@ fn alias() {
         }
       },
       "assignments": {},
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -84,6 +85,7 @@ fn assignment() {
       },
       "first": null,
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {},
       "settings": {
@@ -123,6 +125,7 @@ fn body() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -178,6 +181,7 @@ fn dependencies() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "bar": {
@@ -266,6 +270,7 @@ fn dependency_argument() {
           "depth": 0,
         },
       },
+      "groups": [],
       "modules": {},
       "recipes": {
         "bar": {
@@ -361,6 +366,7 @@ fn duplicate_recipes() {
         }
       },
       "assignments": {},
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -428,6 +434,7 @@ fn duplicate_variables() {
       },
       "first": null,
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {},
       "settings": {
@@ -463,6 +470,7 @@ fn doc_comment() {
       "first": "foo",
       "doc": null,
       "assignments": {},
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -512,6 +520,7 @@ fn empty_justfile() {
       "assignments": {},
       "first": null,
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {},
       "settings": {
@@ -554,6 +563,7 @@ fn parameters() {
       "first": "a",
       "doc": null,
       "assignments": {},
+      "groups": [],
       "modules": {},
       "recipes": {
         "a": {
@@ -707,6 +717,7 @@ fn priors() {
       "assignments": {},
       "first": "a",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "a": {
@@ -792,6 +803,7 @@ fn private() {
       "assignments": {},
       "first": "_foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "_foo": {
@@ -841,6 +853,7 @@ fn quiet() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -902,6 +915,7 @@ fn settings() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -957,6 +971,7 @@ fn shebang() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -1006,6 +1021,7 @@ fn simple() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -1058,6 +1074,7 @@ fn attribute() {
       "assignments": {},
       "first": "foo",
       "doc": null,
+      "groups": [],
       "modules": {},
       "recipes": {
         "foo": {
@@ -1118,12 +1135,107 @@ fn module() {
         "assignments": {},
         "first": null,
         "doc": null,
+        "groups": [],
         "modules": {
           "foo": {
             "aliases": {},
             "assignments": {},
             "first": "bar",
             "doc": "hello",
+            "groups": [],
+            "modules": {},
+            "recipes": {
+              "bar": {
+                "attributes": [],
+                "body": [],
+                "dependencies": [],
+                "doc": null,
+                "name": "bar",
+                "namepath": "foo::bar",
+                "parameters": [],
+                "priors": 0,
+                "private": false,
+                "quiet": false,
+                "shebang": false,
+              }
+            },
+            "settings": {
+              "allow_duplicate_recipes": false,
+              "allow_duplicate_variables": false,
+              "dotenv_filename": null,
+              "dotenv_load": false,
+              "dotenv_path": null,
+              "dotenv_required": false,
+              "export": false,
+              "fallback": false,
+              "positional_arguments": false,
+              "quiet": false,
+              "shell": null,
+              "tempdir" : null,
+              "unstable": false,
+              "ignore_comments": false,
+              "windows_powershell": false,
+              "windows_shell": null,
+            },
+            "unexports": [],
+            "warnings": [],
+          },
+        },
+        "recipes": {},
+        "settings": {
+          "allow_duplicate_recipes": false,
+          "allow_duplicate_variables": false,
+          "dotenv_filename": null,
+          "dotenv_load": false,
+          "dotenv_path": null,
+          "dotenv_required": false,
+          "export": false,
+          "fallback": false,
+          "positional_arguments": false,
+          "quiet": false,
+          "shell": null,
+          "tempdir" : null,
+          "unstable": false,
+          "ignore_comments": false,
+          "windows_powershell": false,
+          "windows_shell": null,
+        },
+        "unexports": [],
+        "warnings": [],
+      }))
+      .unwrap()
+    ))
+    .run();
+}
+
+#[test]
+fn module_group() {
+  Test::new()
+    .justfile(
+      "
+      [group('alpha')]
+      mod foo
+    ",
+    )
+    .tree(tree! {
+      "foo.just": "bar:",
+    })
+    .args(["--dump", "--dump-format", "json"])
+    .stdout(format!(
+      "{}\n",
+      serde_json::to_string(&json!({
+        "aliases": {},
+        "assignments": {},
+        "first": null,
+        "doc": null,
+        "groups": [],
+        "modules": {
+          "foo": {
+            "aliases": {},
+            "assignments": {},
+            "first": "bar",
+            "doc": null,
+            "groups": ["alpha"],
             "modules": {},
             "recipes": {
               "bar": {
diff --git a/tests/list.rs b/tests/list.rs
index 4ba8f707..1dce34fd 100644
--- a/tests/list.rs
+++ b/tests/list.rs
@@ -236,12 +236,13 @@ fn list_with_groups_in_modules() {
     .stdout(
       "
         Available recipes:
-            [FOO]
-            foo
-
+            (no group)
             bar:
                 [BAZ]
                 baz
+
+            [FOO]
+            foo
       ",
     )
     .run();
@@ -396,7 +397,7 @@ fn module_doc_aligned() {
 }
 
 #[test]
-fn space_before_submodules_following_groups() {
+fn submodules_without_groups() {
   Test::new()
     .write("foo.just", "")
     .justfile(
@@ -411,10 +412,11 @@ fn space_before_submodules_following_groups() {
     .stdout(
       "
         Available recipes:
+            (no group)
+            foo ...
+
             [baz]
             bar
-
-            foo ...
       ",
     )
     .run();
diff --git a/tests/modules.rs b/tests/modules.rs
index 97be8a11..7cc23866 100644
--- a/tests/modules.rs
+++ b/tests/modules.rs
@@ -771,6 +771,204 @@ fn doc_attribute_on_module() {
     .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)
+    .arg("--list")
+    .stdout(
+      "
+        Available recipes:
+            (no group)
+            c
+
+            [alpha]
+            a
+            foo ...
+            zee ...
+            
+            [beta]
+            b
+            bar ...
+      ",
+    )
+    .run();
+}
+
+#[test]
+fn group_attribute_on_module_unsorted() {
+  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)
+    .arg("--list")
+    .arg("--unsorted")
+    .stdout(
+      "
+        Available recipes:
+            (no group)
+            c
+
+            [alpha]
+            a
+            zee ...
+            foo ...
+            
+            [beta]
+            b
+            bar ...
+      ",
+    )
+    .run();
+}
+
+#[test]
+fn group_attribute_on_module_list_submodule() {
+  Test::new()
+    .write("foo.just", "d:")
+    .write("bar.just", "e:")
+    .write("zee.just", "f:")
+    .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)
+    .arg("--list")
+    .arg("--list-submodules")
+    .stdout(
+      "
+        Available recipes:
+            (no group)
+            c
+
+            [alpha]
+            a
+            foo:
+                d
+            zee:
+                f
+
+            [beta]
+            b
+            bar:
+                e
+      ",
+    )
+    .run();
+}
+
+#[test]
+fn group_attribute_on_module_list_submodule_unsorted() {
+  Test::new()
+    .write("foo.just", "d:")
+    .write("bar.just", "e:")
+    .write("zee.just", "f:")
+    .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)
+    .arg("--list")
+    .arg("--list-submodules")
+    .arg("--unsorted")
+    .stdout(
+      "
+        Available recipes:
+            (no group)
+            c
+
+            [alpha]
+            a
+            zee:
+                f
+            foo:
+                d
+
+            [beta]
+            b
+            bar:
+                e
+      ",
+    )
+    .run();
+}
+
 #[test]
 fn bad_module_attribute_fails() {
   Test::new()