-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
Support byte grouping #170
Conversation
Thank you for your PR. We recently merged #173 which introduced some large scale changes to the code, so there are a lot of conflicts now. Sorry for that. |
43905a7
to
dfdac73
Compare
I just fix the conflicts. Please take a look again if you may like to accept this change! Thank you. |
@sharifhsn Maybe we should merge this first? What do you think? |
Yes, that's a good idea. I can modify my |
@RinHizakura This looks good from a first review. Can we please add an (integration) test for this new feature? |
Okay. I just merged the clap v4 branch. If you're interested, maybe you could support here with updating the CLI code for the |
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.
These are the only changes that are needed to update for clap
4.
src/bin/hexyl.rs
Outdated
.arg( | ||
Arg::new("group_bytes") | ||
.short('g') | ||
.takes_value(true) |
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.
Replace this option with .num_args(1)
src/bin/hexyl.rs
Outdated
@@ -332,6 +343,25 @@ fn run() -> Result<(), AnyhowError> { | |||
2 | |||
}; | |||
|
|||
let group_bytes = if let Some(group_bytes) = matches | |||
.value_of("group_bytes") |
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.
Replace with .get_one::<String>("group_bytes")
Hi, some of the integration tests are provided now. Please check whether we'll need more tests on the other special patterns which I wasn't come up to. |
src/bin/hexyl.rs
Outdated
The possible option will be 1, 2, 4, 8, otherwise it will be set \ | ||
to 1 by default.", |
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.
The possible option will be 1, 2, 4, 8, otherwise it will be set \ | |
to 1 by default.", | |
The possible options are 1, 2, 4 and 8. The default is 1.", |
src/bin/hexyl.rs
Outdated
if (group_bytes <= 8) && ((group_bytes & (group_bytes - 1)) == 0) { | ||
group_bytes | ||
} else { | ||
1 | ||
} |
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.
I think I would prefer a hard error (in case someone uses an illegal value for group_bytes
), rather than a silent fallback to 1. Or why did you decide to implement it this waay?
tests/integration_tests.rs
Outdated
"┌────────┬─────────────────────┬─────────────────────┬────────┬────────┐\n\ | ||
│00000000│ 3031 3233 3435 3637 ┊ 3839 6162 6364 650a │01234567┊89abcde_│\n\ | ||
└────────┴─────────────────────┴─────────────────────┴────────┴────────┘\n", |
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.
is it possible to format this in the "right" way? or does cargo fmt
destroy it?
@@ -166,6 +166,17 @@ fn run() -> Result<(), AnyhowError> { | |||
based on the current terminal width", | |||
), | |||
) | |||
.arg( | |||
Arg::new("group_bytes") | |||
.short('g') |
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.
Can we please add a long option for this (--group-bytes
) and use it in the unit tests?
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.
Thank you very much. This looks great, except for a few minor comments.
361f07e
to
55f3dfd
Compare
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.
Thank you
This PR is going to support different sizes of data which can almost fit the requirement of #104, without the endian option. Since there's some "magic" number (e.g. the number "25") related to this originally, I also try my best to replace those with a specific abstraction so we can simpler add more features accordingly. To be simple, also note that I make the panel size change dynamically according to the bytes in the group, so the string
No content to print
has to be modified toNo content
to fit the panel when-g 16
. It's fine if you have better ideas about this change.If you think this PR makes sense, I will also add more test cases to robust this feature, thanks!