Merge pull request #129 from popematt/namespace

Refactor namespace logic out of default command logic

src/bin/ion/commands/cat.rs

@@ -16,6 +16,14 @@ impl IonCliCommand for CatCommand {
         "Prints all Ion input files to the specified output in the requested format."
     }
 
+    fn is_stable(&self) -> bool {
+        true
+    }
+
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         command
             .alias("dump")

src/bin/ion/commands/command_namespace.rs

@@ -0,0 +1,84 @@
+use crate::commands::{IonCliCommand, WithIonCliArgument, UNSTABLE_FLAG};
+use clap::{ArgMatches, Command as ClapCommand};
+use std::process;
+
+/// A trait that handles the implementation of [IonCliCommand] for command namespaces.
+pub trait IonCliNamespace {
+    fn name(&self) -> &'static str;
+    fn about(&self) -> &'static str;
+    fn subcommands(&self) -> Vec<Box<dyn IonCliCommand>>;
+}
+
+impl<T: IonCliNamespace> IonCliCommand for T {
+    // Namespaces can't be used on their own, so we'll pretend that they are all stable and
+    // let the leaf commands handle stability.
+    fn is_stable(&self) -> bool {
+        true
+    }
+
+    // Namespaces can't be used on their own, so we'll pretend that they are all plumbing and
+    // let the leaf commands handle plumbing vs porcelain.
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
+    fn name(&self) -> &'static str {
+        IonCliNamespace::name(self)
+    }
+
+    fn about(&self) -> &'static str {
+        IonCliNamespace::about(self)
+    }
+
+    fn configure_args(&self, command: ClapCommand) -> ClapCommand {
+        // Create a `ClapCommand` representing each of this command's subcommands.
+        let clap_subcommands: Vec<_> = self
+            .subcommands()
+            .iter()
+            .map(|s| s.clap_command())
+            .collect();
+
+        let mut command = command
+            .subcommand_required(true)
+            .subcommands(clap_subcommands);
+
+        // If there are subcommands, add them to the configuration and set 'subcommand_required'.
+        let has_unstable_subcommand = self.subcommands().iter().any(|sc| !sc.is_stable());
+        if has_unstable_subcommand {
+            command = command.show_unstable_flag();
+        }
+        command
+    }
+
+    fn run(&self, command_path: &mut Vec<String>, args: &ArgMatches) -> anyhow::Result<()> {
+        // Safe to unwrap because if this is a namespace are subcommands, then clap has already
+        // ensured that a known subcommand is present in args.
+        let (subcommand_name, subcommand_args) = args.subcommand().unwrap();
+        let subcommands = self.subcommands();
+        let subcommand: &dyn IonCliCommand = subcommands
+            .iter()
+            .find(|sc| sc.name() == subcommand_name)
+            .unwrap()
+            .as_ref();
+
+        match (subcommand.is_stable(), args.get_flag(UNSTABLE_FLAG)) {
+            // Warn if using an unnecessary `-X`
+            (true, true) => eprintln!(
+                "'{}' is stable and does not require opt-in",
+                subcommand_name
+            ),
+            // Error if missing a required `-X`
+            (false, false) => {
+                eprintln!(
+                    "'{}' is unstable and requires explicit opt-in",
+                    subcommand_name
+                );
+                process::exit(1)
+            }
+            _ => {}
+        }
+
+        command_path.push(subcommand_name.to_owned());
+        subcommand.run(command_path, subcommand_args)
+    }
+}

src/bin/ion/commands/count.rs

@@ -19,6 +19,10 @@ impl IonCliCommand for CountCommand {
         false
     }
 
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         command.with_input()
     }

src/bin/ion/commands/from/json.rs

@@ -16,11 +16,20 @@ impl IonCliCommand for FromJsonCommand {
     }
 
     fn is_stable(&self) -> bool {
+        false // TODO: Should this be true?
+    }
+
+    fn is_porcelain(&self) -> bool {
         false
     }
 
     fn configure_args(&self, command: Command) -> Command {
-        command.with_input().with_output().with_format()
+        // Args must be identical to CatCommand so that we can safely delegate
+        command
+            .with_input()
+            .with_output()
+            .with_format()
+            .with_ion_version()
     }
 
     fn run(&self, command_path: &mut Vec<String>, args: &ArgMatches) -> Result<()> {

src/bin/ion/commands/from/mod.rs

@@ -1,3 +1,4 @@
+use crate::commands::command_namespace::IonCliNamespace;
 use crate::commands::IonCliCommand;
 
 use crate::commands::from::json::FromJsonCommand;
@@ -6,7 +7,7 @@ pub mod json;
 
 pub struct FromNamespace;
 
-impl IonCliCommand for FromNamespace {
+impl IonCliNamespace for FromNamespace {
     fn name(&self) -> &'static str {
         "from"
     }

src/bin/ion/commands/generate/mod.rs

@@ -30,6 +30,10 @@ impl IonCliCommand for GenerateCommand {
         false
     }
 
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         command
             .arg(

src/bin/ion/commands/head.rs

@@ -16,6 +16,14 @@ impl IonCliCommand for HeadCommand {
         "Prints the specified number of top-level values in the input stream."
     }
 
+    fn is_stable(&self) -> bool {
+        true
+    }
+
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         // Same flags as `cat`, but with an added `--values` flag to specify the number of values to
         // write.

src/bin/ion/commands/inspect.rs

@@ -29,6 +29,10 @@ between versions. Stable output for programmatic use cases is a
 non-goal."
     }
 
+    fn is_stable(&self) -> bool {
+        true
+    }
+
     fn is_porcelain(&self) -> bool {
         true
     }

src/bin/ion/commands/mod.rs

@@ -7,10 +7,10 @@ use clap::builder::ValueParser;
 use clap::{crate_authors, crate_version, Arg, ArgAction, ArgMatches, Command as ClapCommand};
 use std::fs::File;
 use std::io::Write;
-use std::process;
 use termcolor::{ColorChoice, StandardStream, StandardStreamLock};
 
 pub mod cat;
+mod command_namespace;
 pub mod count;
 pub mod from;
 #[cfg(feature = "experimental-code-gen")]
@@ -22,13 +22,15 @@ pub mod schema;
 pub mod symtab;
 pub mod to;
 
+pub(crate) use command_namespace::IonCliNamespace;
+
 /// Behaviors common to all Ion CLI commands, including both namespaces (groups of commands)
 /// and the commands themselves.
 pub trait IonCliCommand {
     /// Indicates whether this command is stable (as opposed to unstable or experimental).
     /// Namespaces should almost always be stable.
     fn is_stable(&self) -> bool {
-        true
+        false
     }
 
     /// Whether the output format is machine-readable.
@@ -39,7 +41,7 @@ pub trait IonCliCommand {
     ///
     /// See https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain#_plumbing_porcelain
     fn is_porcelain(&self) -> bool {
-        false
+        true
     }
 
     /// Returns the name of this command.
@@ -56,13 +58,6 @@ pub trait IonCliCommand {
     /// Commands wishing to customize their `ClapCommand`'s arguments should override
     /// [`Self::configure_args`].
     fn clap_command(&self) -> ClapCommand {
-        // Create a `ClapCommand` representing each of this command's subcommands.
-        let clap_subcommands: Vec<_> = self
-            .subcommands()
-            .iter()
-            .map(|s| s.clap_command())
-            .collect();
-
         // Configure a 'base' clap configuration that has the command's name, about message,
         // version, and author.
 
@@ -97,19 +92,6 @@ pub trait IonCliCommand {
             );
         }
 
-        // If there are subcommands, add them to the configuration and set 'subcommand_required'.
-        if !clap_subcommands.is_empty() {
-            let has_unstable_subcommand = self.subcommands().iter().any(|sc| !sc.is_stable());
-
-            if has_unstable_subcommand {
-                base_command = base_command.show_unstable_flag()
-            }
-
-            base_command = base_command
-                .subcommand_required(true)
-                .subcommands(clap_subcommands)
-        }
-
         self.configure_args(base_command)
     }
 
@@ -120,57 +102,13 @@ pub trait IonCliCommand {
         command
     }
 
-    /// Returns a `Vec` containing all of this command's subcommands.
-    ///
-    /// Namespaces should override the default implementation to specify their subcommands.
-    /// Commands should use the default implementation.
-    fn subcommands(&self) -> Vec<Box<dyn IonCliCommand>> {
-        Vec::new()
-    }
-
-    /// Returns the subcommand that corresponds to the specified name. If no matching subcommand
-    /// is found, returns `None`.
-    fn get_subcommand(&self, subcommand_name: &str) -> Option<Box<dyn IonCliCommand>> {
-        let mut subcommands = self.subcommands();
-        if let Some(index) = subcommands.iter().position(|s| s.name() == subcommand_name) {
-            Some(subcommands.swap_remove(index))
-        } else {
-            None
-        }
-    }
-
     /// The core logic of the command.
     ///
     /// The default implementation assumes this command is a namespace (i.e. a group of subcommands).
     /// It looks for a subcommand in the arguments, then looks up and runs that subcommand.
     ///
     /// Commands should override this implementation.
-    fn run(&self, command_path: &mut Vec<String>, args: &ArgMatches) -> anyhow::Result<()> {
-        // Safe to unwrap because if this is a namespace are subcommands, then clap has already
-        // ensured that a known subcommand is present in args.
-        let (subcommand_name, subcommand_args) = args.subcommand().unwrap();
-        let subcommand = self.get_subcommand(subcommand_name).unwrap();
-
-        match (subcommand.is_stable(), args.get_flag(UNSTABLE_FLAG)) {
-            // Warn if using an unnecessary `-X`
-            (true, true) => eprintln!(
-                "'{}' is stable and does not require opt-in",
-                subcommand_name
-            ),
-            // Error if missing a required `-X`
-            (false, false) => {
-                eprintln!(
-                    "'{}' is unstable and requires explicit opt-in",
-                    subcommand_name
-                );
-                process::exit(1)
-            }
-            _ => {}
-        }
-
-        command_path.push(subcommand_name.to_owned());
-        subcommand.run(command_path, subcommand_args)
-    }
+    fn run(&self, command_path: &mut Vec<String>, args: &ArgMatches) -> anyhow::Result<()>;
 }
 
 /// Argument ID for the '--unstable' / '-X' flag

src/bin/ion/commands/schema/mod.rs

@@ -1,14 +1,15 @@
 pub mod load;
 pub mod validate;
 
+use crate::commands::command_namespace::IonCliNamespace;
 use crate::commands::IonCliCommand;
 
 use crate::commands::schema::load::LoadCommand;
 use crate::commands::schema::validate::ValidateCommand;
 
 pub struct SchemaNamespace;
 
-impl IonCliCommand for SchemaNamespace {
+impl IonCliNamespace for SchemaNamespace {
     fn name(&self) -> &'static str {
         "schema"
     }

src/bin/ion/commands/schema/validate.rs

@@ -28,6 +28,10 @@ impl IonCliCommand for ValidateCommand {
         false
     }
 
+    fn is_porcelain(&self) -> bool {
+        true // TODO: Should this command be made into plumbing?
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         command
             .arg(

src/bin/ion/commands/symtab/filter.rs

@@ -22,6 +22,10 @@ impl IonCliCommand for SymtabFilterCommand {
         false
     }
 
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         command.with_input()
             .with_output()

src/bin/ion/commands/symtab/mod.rs

@@ -1,11 +1,12 @@
+use crate::commands::command_namespace::IonCliNamespace;
 use crate::commands::symtab::filter::SymtabFilterCommand;
 use crate::commands::IonCliCommand;
 
 pub mod filter;
 
 pub struct SymtabNamespace;
 
-impl IonCliCommand for SymtabNamespace {
+impl IonCliNamespace for SymtabNamespace {
     fn name(&self) -> &'static str {
         "symtab"
     }

src/bin/ion/commands/to/json.rs

@@ -25,6 +25,10 @@ impl IonCliCommand for ToJsonCommand {
         false
     }
 
+    fn is_porcelain(&self) -> bool {
+        false
+    }
+
     fn configure_args(&self, command: Command) -> Command {
         // NOTE: it may be necessary to add format-specific options. For example, a "pretty" option
         // would make sense for JSON, but not binary formats like CBOR.

src/bin/ion/commands/to/mod.rs

@@ -1,3 +1,4 @@
+use crate::commands::command_namespace::IonCliNamespace;
 use crate::commands::IonCliCommand;
 
 use crate::commands::to::json::ToJsonCommand;
@@ -6,7 +7,7 @@ pub mod json;
 
 pub struct ToNamespace;
 
-impl IonCliCommand for ToNamespace {
+impl IonCliNamespace for ToNamespace {
     fn name(&self) -> &'static str {
         "to"
     }