Skip to content
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

feat: add skip option #88

Merged
merged 4 commits into from
May 13, 2020
Merged

feat: add skip option #88

merged 4 commits into from
May 13, 2020

Conversation

Tarnadas
Copy link
Contributor

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
Copy link
Contributor Author

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

@sharkdp
Copy link
Owner

sharkdp commented Apr 30, 2020

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.

src/input.rs Outdated Show resolved Hide resolved
@sharkdp
Copy link
Owner

sharkdp commented Apr 30, 2020

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
Copy link
Contributor Author

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.

@Tarnadas Tarnadas requested a review from sharkdp May 12, 2020 04:51
src/input.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
@Tarnadas Tarnadas requested a review from sharkdp May 13, 2020 15:53
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sharkdp sharkdp merged commit 5327b71 into sharkdp:master May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options for reading between two offsets
2 participants