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

Allow hex numbers and data units for block size argument #144

Merged

Conversation

merkrafter
Copy link
Contributor

@merkrafter merkrafter commented Nov 7, 2021

Summary

Closes #111
This PR allows arguments like 4kb or 0x10 for --block-size.
The 'block' unit is not allowed in this context (see discussion on related issue).

@sharkdp
Copy link
Owner

sharkdp commented Nov 10, 2021

Thank you very much for your contribution!

This looks great on a first glance. Happy to review it in detail after the cleanup.

I used the const_format crate to display the default block size to the user in a convenient and consistent way. However, using it required Rust 1.46.0+. Do you think this detail is worth raising the minimum required Rust version?

I didn't know this crate. That sounds really helpful. Bumping to Rust 1.46 sounds good. Since this only affects hexyl as a tool and not hexyl as a library, I'd be okay to bump it even further, in case that helps with other parts of the code. I don't think we have a lot of "customers" anyway: https://crates.io/crates/hexyl/reverse_dependencies

@merkrafter merkrafter marked this pull request as ready for review November 10, 2021 22:04
@merkrafter
Copy link
Contributor Author

Please let me know if you have other concerns/ideas/suggestions etc. :))

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 very much. Great work on the refactoring - this looks much cleaner now! I looked closely but I don't have any additional comments ;-)

@sharkdp sharkdp merged commit d1ae685 into sharkdp:master Nov 14, 2021
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.

Argument --block-size does not accept units (or hex numbers), contrary to its helptext description.
2 participants