search

sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

feat: add skip option #88

Closed Tarnadas closed 4 years ago

Tarnadas commented 4 years ago

Since PR #75 has been open for quite some time now, I wanted to implement my own skip option. This PR uses the Seek trait as suggested.

resolves #16 and #87

Tarnadas commented 4 years ago

@sharkdp Should I try to make it compile on Rust 1.31 or would you rather like to increase minimum required Rust version?

sharkdp commented 4 years ago

Thank you for your contribution. I bumped the min. required Rust version to 1.36 and "backported" it to this version (by matching on Input::* instead of Self::*).

I also removed the RefCell layer as it is not needed.

sharkdp commented 4 years ago

This will also fail if the input is not a file, but a pipe, for example (this is not "reading from STDIN"):

❯ hexyl <(echo test) --skip 2
Error: Illegal seek (os error 29)

Maybe we could catch that specific OS error and print a similar error message.

Another thing: when using --skip N, shouldn't the display offset be set to N? (unless the user explicitly specifies it).

What about the --range N:M idea in #16 and #75? Do you think it would be compatible with your approach? Could it be implemented in a follow up PR?

Tarnadas commented 4 years ago

I changed the error message to what you suggested. It will now also catch this specific OS error and display a proper error message.

You're right about the display offset, so it will now use skip as a default, if the user does not specifiy it.

It should still be possible to implement the --range N:M option. I think it would even make it easier, because you basically just have to parse it and set skip and length respectively.