Promotes head, improves inspect
#118
Conversation
* `head` is no longer in the `beta` namespace * Bumps `ion_rs` dependency to v1.0.0-rc.6 * `inspect` confirms its input is a supported encoding * Fixes a bug in symbol ID assignments * Removes workaround for `body_span` now-fixed bug in `ion_rs` * Makes `--no-auto-decompress` a supported-by-default flag for all subcommands. * Hides the `dump` alias for `cat` from `--help`'s list of subcommands.
Cargo.toml
Outdated
| @@ -17,7 +17,7 @@ clap = { version = "4.0.17", features = ["cargo"] } | |||
| colored = "2.0.0" | |||
| flate2 = "1.0" | |||
| infer = "0.15.0" | |||
| ion-rs = { version = "1.0.0-rc.5", features = ["experimental"] } | |||
| ion-rs = { version = "1.0.0-rc.6", git = "https://github.com/amazon-ion/ion-rust.git", branch = "version-bump-rc6", features = ["experimental"] } | |||
There was a problem hiding this comment.
🗺️ This PR depends on amazon-ion/ion-rust#785. Once that's merged/released, I'll update this to point to crates.io.
| @@ -21,7 +21,6 @@ impl IonCliCommand for SymtabFilterCommand { | |||
| fn configure_args(&self, command: Command) -> Command { | |||
| command.with_input() | |||
| .with_output() | |||
| .with_compression_control() | |||
There was a problem hiding this comment.
🗺️ Prior to this PR, commands needed to explicitly opt into supporting the --no-auto-decompress flag. Forgetting to do so would cause the program to ugly-crash when the flag was specified. Now the flag is supported universally by default.
| fn confirm_encoding_is_supported(&self, encoding: IonEncoding) -> Result<()> { | ||
| use IonEncoding::*; | ||
| match encoding { | ||
| Text_1_0 | Text_1_1 => { | ||
| bail!("`inspect` does not support text Ion streams."); | ||
| } | ||
| Binary_1_0 => Ok(()), | ||
| Binary_1_1 => bail!("`inspect` does not yet support binary Ion v1.1"), | ||
| // `IonEncoding` is #[non_exhaustive] | ||
| _ => bail!("`inspect does not yet support {}", encoding.name()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗺️ The latest ion_rs makes it possible to get the IonEncoding from any raw stream item. This allows us to trivially confirm that we're working with what we expect; in inspect's case, that's binary Ion 1.0.
| /// Iterates over the items in `reader`, printing a table section for each top level value. | ||
| fn inspect_top_level<Input: IonInput>( | ||
| &mut self, | ||
| reader: &mut SystemReader<AnyEncoding, Input>, | ||
| ) -> Result<()> { | ||
| const TOP_LEVEL_DEPTH: usize = 0; | ||
| self.write_table_header()?; |
There was a problem hiding this comment.
🗺️ We now defer writing the table header until after we've read the first item from the stream and confirmed it's of a supported encoding.
| let is_append = lazy_struct.get("imports")? | ||
| == Some(ValueRef::Symbol(SymbolRef::with_text("$ion_symbol_table"))); | ||
| if !is_append { | ||
| next_symbol_id = 10; // First available SID after system symbols in Ion 1.0 | ||
| } | ||
| self.inspect_symbol_table(next_symbol_id, lazy_struct)?; |
There was a problem hiding this comment.
🗺️ Now that a LazyValue can return a reference to its SymbolTable, the symbol ID assignment logic can be localized to the method where it's displayed.
| // TODO: This does not account for shared symbol table imports. However, the CLI does not | ||
| // yet support specifying a catalog, so it's correct enough for the moment. | ||
| let symtab_value = symtab_struct.as_value(); | ||
| let mut next_symbol_id = symtab_value.symbol_table().len(); |
There was a problem hiding this comment.
🗺️ We don't have to track which ID we were on because we can ask the symbol table what the next ID will be.
| BytesKind::ValueBody, | ||
| &encoding.span().bytes()[total_len - body_len..], | ||
| ); | ||
| let body_bytes = IonBytes::new(BytesKind::ValueBody, encoding.body_span().bytes()); |
There was a problem hiding this comment.
🗺️ ion_rs fixed the bug in body_span(), so I was able to simplify this.
| if let Some(flag) = self.args.get_one::<bool>("no-auto-decompress") { | ||
| !*flag | ||
| if let Some(is_disabled) = self.args.get_one::<bool>("no-auto-decompress") { | ||
| !*is_disabled |
There was a problem hiding this comment.
🗺️ This is just a naming change for clarity.
| Box::new(CatCommand), | ||
| Box::new(DumpCommand), | ||
| Box::new(HeadCommand), |
There was a problem hiding this comment.
🗺️ I put the commands in alphabetical order while I was here.
| @@ -201,7 +201,6 @@ fn test_write_all_values(#[case] number: i32, #[case] expected_output: &str) -> | |||
| input_file.write_all(test_data.as_bytes())?; | |||
| input_file.flush()?; | |||
| cmd.args([ | |||
| "beta", | |||
There was a problem hiding this comment.
🗺️ This unit test invoked ion beta head and needed to be updated to ion head.
|
The CI checks are failing because the |
headout of thebetanamespaceinspect:body_spannow-fixed bug inion_rs--no-auto-decompressa supported-by-default flag forall subcommands.
dumpalias forcatfrom--help's list of subcommands.ion_rsdependency to v1.0.0-rc.6By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.