Makes to and from into namespaces with a nested json command (#54)

Prior to this PR, `to` and `from` were commands that each took a `format`
flag that could be used to specify `json`. This patch promotes `to` and
`from` into namespaces that each have a nested `json` command. This makes
it easier to add support for other subcommands in `to` and `from`, and
allows those subcommands to expose settings that only make sense for the
corresponding format. (For example: configuring a catalog for Ion data,
or a list of supported tags for CBOR.)

Also fixes #36 by manually outputting a newline after writing any file
in an Ion text format.

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

@@ -1,14 +1,13 @@
 use crate::commands::dump;
-use anyhow::{Context, Result};
+use anyhow::Result;
 use clap::{Arg, ArgAction, ArgMatches, Command};
 
-const ABOUT: &str = "Converts data from a particular format into Ion. Currently supports json.";
+const ABOUT: &str = "Converts data from JSON to Ion.";
 
 // Creates a `clap` (Command Line Arguments Parser) configuration for the `from` command.
-// This function is invoked by the `from` command's parent, `beta`, so it can describe its
-// child commands.
+// This function is invoked by the parent command,`from`, so it can describe its child commands.
 pub fn app() -> Command {
-    Command::new("from")
+    Command::new("json")
         .about(ABOUT)
         .arg(
             Arg::new("output")
@@ -24,18 +23,11 @@ pub fn app() -> Command {
                 .value_parser(["binary", "text", "pretty", "lines"])
                 .help("Output format"),
         )
-        .arg(
-            Arg::new("source_format")
-                .index(1)
-                .help("Format of the data to convert."),
-        )
         .arg(
             // Any number of input files can be specified by repeating the "-i" or "--input" flags.
             // Unlabeled positional arguments will also be considered input file names.
             Arg::new("input")
-                .long("input")
-                .short('i')
-                .index(2)
+                .index(1)
                 .trailing_var_arg(true)
                 .action(ArgAction::Append)
                 .help("Input file"),
@@ -44,19 +36,7 @@ pub fn app() -> Command {
 
 // This function is invoked by the `from` command's parent, `beta`.
 pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
-    let result = match matches
-        .get_one::<String>("source_format")
-        .with_context(|| "No `source_format` was specified.")?
-        .as_str()
-    {
-        "json" => {
-            // Because JSON data is valid Ion, the `dump` command may be reused for converting JSON.
-            // TODO ideally, this would perform some smarter "up-conversion".
-            dump::run("from", matches)
-        }
-        _ => {
-            unimplemented!("Unsupported format.")
-        }
-    };
-    result
+    // Because JSON data is valid Ion, the `dump` command may be reused for converting JSON.
+    // TODO ideally, this would perform some smarter "up-conversion".
+    dump::run("json", matches)
 }

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

@@ -0,0 +1,42 @@
+use crate::commands::CommandRunner;
+use anyhow::Result;
+use clap::{ArgMatches, Command};
+
+pub mod json;
+
+// Creates a Vec of CLI configurations for all of the available built-in commands
+pub fn subcommands() -> Vec<Command> {
+    vec![json::app()]
+}
+
+// Maps the given command name to the entry point for that command if it exists
+pub fn runner_for_from_command(command_name: &str) -> Option<CommandRunner> {
+    let runner = match command_name {
+        "json" => json::run,
+        _ => return None,
+    };
+    Some(runner)
+}
+
+// The functions below are used by the parent `beta` command when `to` is invoked.
+pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
+    //     ^-- At this level of dispatch, this command will always be the text `to`.
+    // We want to evaluate the name of the subcommand that was invoked --v
+    let (command_name, command_args) = matches.subcommand().unwrap();
+    if let Some(runner) = runner_for_from_command(command_name) {
+        runner(command_name, command_args)?;
+    } else {
+        let message = format!(
+            "The requested `from` command ('{}') is not supported and clap did not generate an error message.",
+            command_name
+        );
+        unreachable!("{}", message);
+    }
+    Ok(())
+}
+
+pub fn app() -> Command {
+    Command::new("from")
+        .about("'from' is a namespace for commands that converts other data formats to Ion.")
+        .subcommands(subcommands())
+}

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

@@ -2,53 +2,37 @@ use anyhow::{Context, Result};
 use clap::{Arg, ArgAction, ArgMatches, Command};
 use ion_rs::element::reader::ElementReader;
 use ion_rs::element::Element;
-use ion_rs::ReaderBuilder;
-use memmap::MmapOptions;
+use ion_rs::{Reader, ReaderBuilder};
 use serde_json::{Map, Number, Value as JsonValue};
 use std::fs::File;
-use std::io;
-use std::io::{stdout, BufWriter, Write};
+use std::io::{stdin, stdout, BufWriter, Write};
 use std::str::FromStr;
 
-const ABOUT: &str = "Converts data from Ion into a requested format. Currently supports json.";
+const ABOUT: &str = "Converts Ion data to JSON.";
 
-// Creates a `clap` (Command Line Arguments Parser) configuration for the `to` command.
-// This function is invoked by the `to` command's parent, `beta`, so it can describe its
-// child commands.
 pub fn app() -> Command {
-    Command::new("to")
+    Command::new("json")
         .about(ABOUT)
         .arg(
             Arg::new("output")
                 .long("output")
                 .short('o')
                 .help("Output file [default: STDOUT]"),
         )
-        .arg(Arg::new("format").index(1).help("Output format"))
         .arg(
-            // Any number of input files can be specified by repeating the "-i" or "--input" flags.
             // Unlabeled positional arguments will also be considered input file names.
             Arg::new("input")
-                .long("input")
-                .short('i')
-                .index(2)
+                .index(1)
                 .trailing_var_arg(true)
                 .action(ArgAction::Append)
-                .help("Input file"),
+                .help("Input file name"),
         )
     // 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.
 }
 
 pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
-    // NOTE: the following logic is copied from inspect.run(), and should be refactored for reuse.
-
-    let format = matches
-        .get_one::<String>("format")
-        .with_context(|| "No `format` was specified.")?
-        .as_str();
-
-    // -o filename
+    // Look for an output file name specified with `-o`
     let mut output: Box<dyn Write> = if let Some(output_file) = matches.get_one::<String>("output")
     {
         let file = File::create(output_file).with_context(|| {
@@ -57,75 +41,44 @@ pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
                 output_file
             )
         })?;
-        Box::new(file)
+        Box::new(BufWriter::new(file))
     } else {
         Box::new(stdout().lock())
     };
 
-    if let Some(input_file_iter) = matches.get_many::<String>("input") {
-        for input_file in input_file_iter {
-            let mut file = File::open(input_file)
-                .with_context(|| format!("Could not open file '{}'", input_file))?;
-            convert(&mut file, &mut output, format)?;
+    if let Some(input_file_names) = matches.get_many::<String>("input") {
+        // Input files were specified, run the converter on each of them in turn
+        for input_file in input_file_names {
+            let file = File::open(input_file.as_str())
+                .with_context(|| format!("Could not open file '{}'", &input_file))?;
+            let mut reader = ReaderBuilder::new()
+                .build(file)
+                .with_context(|| format!("Input file {} was not valid Ion.", &input_file))?;
+            convert(&mut reader, &mut output)?;
         }
     } else {
-        // If no input file was specified, run the inspector on STDIN.
-
-        // The inspector expects its input to be a byte array or mmap()ed file acting as a byte
-        // array. If the user wishes to provide data on STDIN, we'll need to copy those bytes to
-        // a temporary file and then read from that.
-
-        // Create a temporary file that will delete itself when the program ends.
-        let mut input_file = tempfile::tempfile().with_context(|| {
-            concat!(
-                "Failed to create a temporary file to store STDIN.",
-                "Try passing an --input flag instead."
-            )
-        })?;
-
-        // Pipe the data from STDIN to the temporary file.
-        let mut writer = BufWriter::new(input_file);
-        io::copy(&mut io::stdin(), &mut writer)
-            .with_context(|| "Failed to copy STDIN to a temp file.")?;
-        // Get our file handle back from the BufWriter
-        input_file = writer
-            .into_inner()
-            .with_context(|| "Failed to read from temp file containing STDIN data.")?;
-        convert(&mut input_file, &mut output, format)?;
+        // No input files were specified, run the converter on STDIN.
+        let mut reader = ReaderBuilder::new()
+            .build(stdin().lock())
+            .with_context(|| "Input was not valid Ion.")?;
+        convert(&mut reader, &mut output)?;
     }
 
     output.flush()?;
     Ok(())
 }
 
-pub fn convert(file: &mut File, output: &mut Box<dyn Write>, format: &str) -> Result<()> {
-    // NOTE: mmap logic is copied from inspect.inspect_file().
-
-    // mmap involves operating system interactions that inherently place its usage outside of Rust's
-    // safety guarantees. If the file is unexpectedly truncated while it's being read, for example,
-    // problems could arise.
-    let mmap = unsafe {
-        MmapOptions::new()
-            .map(file)
-            .with_context(|| "Could not mmap ")?
-    };
-
-    // Treat the mmap as a byte array.
-    let ion_data: &[u8] = &mmap[..];
-    let mut reader = ReaderBuilder::new()
-        .build(ion_data)
-        .with_context(|| "No `source_format` was specified.")?;
-    match format {
-        "json" => {
-            for result in reader.elements() {
-                let element = result.with_context(|| "invalid input")?;
-                writeln!(output, "{}", to_json_value(&element)?)?
-            }
+pub fn convert(reader: &mut Reader, output: &mut Box<dyn Write>) -> Result<()> {
+    const FLUSH_EVERY_N: usize = 100;
+    let mut element_count = 0usize;
+    for result in reader.elements() {
+        let element = result.with_context(|| "invalid input")?;
+        writeln!(output, "{}", to_json_value(&element)?)?;
+        element_count += 1;
+        if element_count % FLUSH_EVERY_N == 0 {
+            output.flush()?;
         }
-        _ => {
-            unimplemented!("Unsupported format.")
-        }
-    };
+    }
     Ok(())
 }
 

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

@@ -0,0 +1,42 @@
+use crate::commands::CommandRunner;
+use anyhow::Result;
+use clap::{ArgMatches, Command};
+
+pub mod json;
+
+// Creates a Vec of CLI configurations for all of the available built-in commands
+pub fn subcommands() -> Vec<Command> {
+    vec![json::app()]
+}
+
+// Maps the given command name to the entry point for that command if it exists
+pub fn runner_for_to_command(command_name: &str) -> Option<CommandRunner> {
+    let runner = match command_name {
+        "json" => json::run,
+        _ => return None,
+    };
+    Some(runner)
+}
+
+// The functions below are used by the parent `beta` command when `to` is invoked.
+pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
+    //     ^-- At this level of dispatch, this command will always be the text `to`.
+    // We want to evaluate the name of the subcommand that was invoked --v
+    let (command_name, command_args) = matches.subcommand().unwrap();
+    if let Some(runner) = runner_for_to_command(command_name) {
+        runner(command_name, command_args)?;
+    } else {
+        let message = format!(
+            "The requested `to` command ('{}') is not supported and clap did not generate an error message.",
+            command_name
+        );
+        unreachable!("{}", message);
+    }
+    Ok(())
+}
+
+pub fn app() -> Command {
+    Command::new("to")
+        .about("'to' is a namespace for commands that convert Ion to another data format.")
+        .subcommands(subcommands())
+}

src/bin/ion/commands/dump.rs

@@ -66,11 +66,6 @@ pub fn run(_command_name: &str, matches: &ArgMatches) -> Result<()> {
     };
 
     if let Some(input_file_iter) = matches.get_many::<String>("input") {
-        //TODO: Hack around newline issue, append newline after `pretty` and `lines`, single newline
-        // at the end of *all* `text` value output (if handling multiple files)
-        //TODO: Solve these newline issues, get rid of hack
-        // https://github.com/amazon-ion/ion-cli/issues/36
-        // https://github.com/amazon-ion/ion-rust/issues/437
         for input_file in input_file_iter {
             let file = File::open(input_file)
                 .with_context(|| format!("Could not open file '{}'", input_file))?;
@@ -95,18 +90,30 @@ pub(crate) fn write_in_format(
     format: &str,
     count: Option<usize>,
 ) -> IonResult<usize> {
-    match format {
+    // XXX: The text formats below each have additional logic to append a newline because the
+    //      ion-rs writer doesn't handle this automatically like it should.
+    //TODO: Solve these newline issues, get rid of hack
+    // https://github.com/amazon-ion/ion-cli/issues/36
+    // https://github.com/amazon-ion/ion-rust/issues/437
+    const NEWLINE: u8 = 0x0A;
+    let written = match format {
         "pretty" => {
             let mut writer = TextWriterBuilder::pretty().build(output)?;
-            transcribe_n_values(reader, &mut writer, count)
+            let values_written = transcribe_n_values(reader, &mut writer, count)?;
+            writer.output_mut().write_all(&[NEWLINE])?;
+            Ok(values_written)
         }
         "text" => {
             let mut writer = TextWriterBuilder::default().build(output)?;
-            transcribe_n_values(reader, &mut writer, count)
+            let values_written = transcribe_n_values(reader, &mut writer, count)?;
+            writer.output_mut().write_all(&[NEWLINE])?;
+            Ok(values_written)
         }
         "lines" => {
             let mut writer = TextWriterBuilder::lines().build(output)?;
-            transcribe_n_values(reader, &mut writer, count)
+            let values_written = transcribe_n_values(reader, &mut writer, count)?;
+            writer.output_mut().write_all(&[NEWLINE])?;
+            Ok(values_written)
         }
         "binary" => {
             let mut writer = BinaryWriterBuilder::new().build(output)?;
@@ -116,7 +123,8 @@ pub(crate) fn write_in_format(
             "'format' was '{}' instead of 'pretty', 'text', 'lines', or 'binary'",
             unrecognized
         ),
-    }
+    }?;
+    Ok(written)
 }
 
 /// Writes each value encountered in the Reader to the provided IonWriter. If `count` is specified