-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds --hex option to inspect #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! Might be nice to also support 0xXX
notation and commas to more easily support copy/paste from sources of byte information, but that's a problem for another day.
src/bin/ion/commands/inspect.rs
Outdated
if read_as_hex_string { | ||
inspect_input( | ||
&input_name, | ||
IonStream::new(HexReader::from(input)), | ||
output, | ||
bytes_to_skip, | ||
limit_bytes, | ||
hide_expansion, | ||
) | ||
} else { | ||
inspect_input( | ||
&input_name, | ||
input, | ||
output, | ||
bytes_to_skip, | ||
limit_bytes, | ||
hide_expansion, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ELI5 why can't we define a single variable for IonStream::new(HexReader::from(input))
/input
? Both are of type Input
but have different... sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR they actually have different concrete types. One is HexReader<AutoDecompressingReader>
and the other one is AutoDecompressingReader
. That means that the function call has to have the type parameters resolved in different ways.
Longer story... AutoDecompressingReader
is actually BufReader<Box<dyn Read>>
so if I could easily inject this earlier in the process, then I could hide the HexReader
inside the Box<dyn Read>
. However, we don't necessarily want to add this hex
option to all of the commands, and I want to continue re-using the existing CommandIo
for managing most of the IO, so I'm injecting it later in the process. I could also make them fit into a single variable if I assigned them to another Box<dyn Read>
, but I don't want to introduce two layers of dyn Trait
indirection when there's only two possible types here. Finally, I could introduce an enum that unifies these two types, but again, that seems like overkill for this one case where we're just selecting between two possible implementations of Read
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, thank you for bringing this up. It made me realize that I could clean things up by implementing the IonInput
trait for HexReader
.
Issue #, if available:
None.
Description of changes:
The
--hex
option can act as a flag, tellinginspect
to assume that the input is a sequence of hexadecimal pairs. It can also accept a value (e.g.--hex="e0 01 00 ea"
), in which case it will ignore any other input and inspect the value given for the hex option.To support this...
CommandIo
has been updated to allow usingCommandOutput
without also usingCommandInput
HexReader
that wraps another reader and translates from hex digit string tou8
s.Other changes...
wrap-help
in a prior PR, so I reformatted all the help text ininspect
to take advantage of the automatic wrapping.primitive
command.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.