hexyl icon indicating copy to clipboard operation
hexyl copied to clipboard

Add more formatting options

Open Aaron-Rumpler opened this issue 3 years ago • 9 comments

Adds the following options:

  • --hex-inner-separator none Removes the inner separator from the hex display
  • --text-inner-separator none Removes the inner separator from the text display
  • --outer-border none Removes the outer border (including header, footer and left border, but currently leaves right border in)

TODO:

  • [ ] Confirm naming of options and associated variables, structs etc.
  • [ ] Confirm documentation in --help for new options
  • [ ] Add changes to CHANGELOG.MD
  • [ ] Add tests for new options
  • [x] Check existing tests haven't broken

Aaron-Rumpler avatar Jan 19 '22 09:01 Aaron-Rumpler

Thank you for your contribution. Could you explain the motivation for this change in a bit more detail? And maybe show a few screenshots how the modified output looks like? Or is there an open ticket that discusses this?

sharkdp avatar Jan 23 '22 11:01 sharkdp

The primary motivation is that I just find the inner separator on the text column kind of annoying sometimes. I also wanted to be able to output without the top and bottom borders (so that line 1 of the output would correspond to the first line of the actual hex dump), but still keep the separator lines.

Aaron-Rumpler avatar Jan 24 '22 09:01 Aaron-Rumpler

hexyl hex text hex_text outer_border hex_text_outer_border

Aaron-Rumpler avatar Jan 24 '22 10:01 Aaron-Rumpler

Only just realised, but combining --hex-inner-separator none, --text-inner-separator none and --outer-border none results in an output very similar to hexdump -C: hexdump

Aaron-Rumpler avatar Jan 24 '22 10:01 Aaron-Rumpler

Thank you very much for the explanation. I'm inclined to accept a change like this, but I would like if we could think about the command-line interface a bit. For example: is there any way we could avoid introducing multiple new options for this?

I also have some questions when looking at the screenshots:

hex_text

should --hex-inner-separator none also get rid of the longer whitespace separator between the left and right hex block? to be consistent with --text-inner-separator none?

outer_border

shouldn't this remove the right border as well?

sharkdp avatar Jan 30 '22 20:01 sharkdp

I would like if we could think about the command-line interface a bit. For example: is there any way we could avoid introducing multiple new options for this?

I agree wrt the command line interface, which is the primary reason this is still a draft PR.

hex_text

should --hex-inner-separator none also get rid of the longer whitespace separator between the left and right hex block? to be consistent with --text-inner-separator none?

The argument against this would be hexdump -C, which also has the double space gap between the left and right hex block, but no gap on the text.

hexdump

outer_border

shouldn't this remove the right border as well?

With the current spacing, I found doing that to look quite unbalanced.

hex_text_outer_border_no_right_pipe

We could do something like this:

hex_text_outer_border_no_right_pipe_new_spacing

There is an issue with not having a right border though (which also affects --border none). With input like this it's hard to tell from the text alone that there's a space there.

hex_text_outer_border_no_right_pipe_issue

Again, hexdump -C has a left and right border on the text, even though it has no other borders.

hexdump_trailing_space_line

Aaron-Rumpler avatar Jan 31 '22 06:01 Aaron-Rumpler

should --hex-inner-separator none also get rid of the longer whitespace separator between the left and right hex block? to be consistent with --text-inner-separator none?

The argument against this would be hexdump -C, which also has the double space gap between the left and right hex block, but no gap on the text.

okay :+1:

shouldn't this remove the right border as well?

With the current spacing, I found doing that to look quite unbalanced.

Okay, so maybe we rather use something like --outer-border=none/left/right/both and let the user decide?

There is an issue with not having a right border though (which also affects --border none). With input like this it's hard to tell from the text alone that there's a space there.

Good point. Any suggestions?

Regarding the CLI: can we write down some alternative proposals and comare them? Feel free to also suggest breaking changes (like a reuse of the existing --border option).

sharkdp avatar Feb 14 '22 07:02 sharkdp

There is an issue with not having a right border though (which also affects --border none). With input like this it's hard to tell from the text alone that there's a space there.

Good point. Any suggestions?

We could print a trailing symbol (like ␄, though there are probably much better options) after the end of the file. There is the case where it ends up on a new line, in which case you just wouldn't show it, as the input ends at the end of a line.

hexdump -C does print a trailing line that includes the total input size, which might be a good idea to offer. In that case, you'd print the ␄ regardless.

hexdump_trailing_space_line

Though talking about the ␄ character, there could be an option to display those for control characters (it's not like you could confuse those for that character appearing in the input itself, as it's Unicode).

Aaron-Rumpler avatar May 03 '22 04:05 Aaron-Rumpler

shouldn't this remove the right border as well?

With the current spacing, I found doing that to look quite unbalanced.

Okay, so maybe we rather use something like --outer-border=none/left/right/both and let the user decide?

Regarding the CLI: can we write down some alternative proposals and comare them? Feel free to also suggest breaking changes (like a reuse of the existing --border option).

I'm partial to replacing --border with something more like what bat does for --style. Could have left, right, top, bottom, hex-inner, text-inner. none would simply disable all the borders, and there'd be options for enabling and disabling groups as well. I think the ASCII/Unicode option should be called something different, as it affects more than just the borders.

Aaron-Rumpler avatar May 03 '22 04:05 Aaron-Rumpler