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. |
RinHizakura
force-pushed
the
master
branch
4 times, most recently
from
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 |
src/bin/hexyl.rs
Outdated
| .arg( | ||
| Arg::new("group_bytes") | ||
| .short('g') | ||
| .takes_value(true) |
There was a problem hiding this comment.
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.
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.
| 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.
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.
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.
Can we please add a long option for this (--group-bytes) and use it in the unit tests?
RinHizakura
force-pushed
the
master
branch
2 times, most recently
from
361f07e
to
55f3dfd
Compare
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 printhas to be modified toNo contentto 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!