Makes to and from into namespaces with a nested json command#54
Makes to and from into namespaces with a nested json command#54
to and from into namespaces with a nested json command#54Conversation
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.
zslayton
changed the title
to and from into namespaces with a nested json command
There was a problem hiding this comment.
🗺️ There are still a LOT of opportunities to DRY up this codebase with some traits to capture the common dispatch logic and base application configuration flags, but they're out of scope for this PR.
| .long("input") | ||
| .short('i') |
There was a problem hiding this comment.
🗺️ At some point certain subcommands began accepting BOTH -i input files and trailing arguments as input files. This is reasonable, but it isn't supported as a first-class feature in clap. clap has a debug_assert that will cause the program to panic in non---release configurations if an option is both named (-i, --input) and positional.
For now, I'm removing the named flag. We can add it back later, but it will take a bit more code to achieve in the prescribed manner.
| ) | ||
| })?; | ||
| Box::new(file) | ||
| Box::new(BufWriter::new(file)) |
There was a problem hiding this comment.
🗺️ Previously the to json command was not using buffered output except when writing to STDOUT.
| }; | ||
|
|
||
| // Treat the mmap as a byte array. | ||
| let ion_data: &[u8] = &mmap[..]; |
There was a problem hiding this comment.
🗺️ ion-rs now supports iterating over the elements in a buffered reader, so we no longer have to mmap the input file. This also means that we can read directly from STDIN without first dumping it to a temporary file.
There was a problem hiding this comment.
🗺️ Again, lots of opportunity to DRY this up remains.
| ) -> 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. |
There was a problem hiding this comment.
🗺️ Addresses #36. The real fix still needs to happen upstream in ion-rs. See amazon-ion/ion-rust#437.
desaikd
left a comment
desaikd
left a comment
There was a problem hiding this comment.
Looks good! It would be nice to have a parent sub command (maybe convert) for from and to. I think that will give us nice encapsulation of the converter commands as well as we could specify some common behavior there but as you mentioned this is out of scope for the PR.
| }; | ||
| 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". |
There was a problem hiding this comment.
Even more ideally, this smarter up-conversion would be configurable.
| // 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> { |
There was a problem hiding this comment.
Some day... #1
The factoring here allows for discovery of an external command or subcommand in runner_for_FOO_command- I like it. It would be even better if the interface included the known command context so far, e.g. here it would be ["ion", "from"] so that e.g. external command discovery could look for ion-from-cbor and know that ion from cbor should invoke it.
Thoughts? This comment might be better in #1 than here... added as a comment there.
| 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 { |
There was a problem hiding this comment.
We see this pattern repeated over and over again in ion-cli- in the long term it would be great if we had a common solution to it. Nothing to do here at the moment.
3 participants
Prior to this PR,
toandfromwere commands that each took aformatflag that could be used to specify
json. This patch promotestoandfrominto namespaces that each have a nestedjsoncommand. This makesit easier to add support for other subcommands in
toandfrom, andallows 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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.