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

Add more formatting options #149

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Aaron-Rumpler
Copy link

@Aaron-Rumpler Aaron-Rumpler commented Jan 19, 2022

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
  • Check existing tests haven't broken

@sharkdp
Copy link
Owner

sharkdp commented Jan 23, 2022

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?

@Aaron-Rumpler
Copy link
Author

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

hexyl
hex
text
hex_text
outer_border
hex_text_outer_border

@Aaron-Rumpler
Copy link
Author

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

@sharkdp
Copy link
Owner

sharkdp commented Jan 30, 2022

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?

@Aaron-Rumpler
Copy link
Author

Aaron-Rumpler commented Jan 31, 2022

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

@sharkdp
Copy link
Owner

sharkdp commented Feb 14, 2022

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 👍

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).

@Aaron-Rumpler
Copy link
Author

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

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.

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.

2 participants