sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

Argument `--length=0`/`--bytes=0` should be considered an error. #106

Closed eth-p closed 3 years ago

eth-p commented 3 years ago

This one might be a bit pedantic, but a user requesting that hexyl print nothing seems like a user error. The entire point of hexyl is to print a binary preview of a file, and explicitly asking it not to defeats the purpose of using it in the first place.


Note: I'm doing a class assignment regarding creating tests (from scratch) for an open-source command line program, and I chose hexyl for it. You might have a couple more of these on the way, depending on what the assignment brings to light :)

sharkdp commented 3 years ago

Hm. I'm not sure if there is an actual use case for that, but it seems unlikely that someone uses --length=0 by accident and would be surprised by the behavior. Someone might call hexyl from a script which automatically computes a value for --length where 0 could be a valid possibility (maybe when running several hexyl calls in succession). Even if unlikely, I'm not sure if we should artificially restrict a perfectly valid execution of hexyl.

eth-p commented 3 years ago

That's fair, I suppose scripts might have some use for it that I haven't considered. I'm honestly just not a fan of printing an empty grid without providing an explanation for it, though. It comes off as slightly unintuitive, especially when computed through a script where I can't immediately see "oh, I set the length to 0".

As a solution, maybe a (disableable) warning could be shown rather than printing an empty grid?

Or it could probably be addressed in a more generic way by adding a header or indicator somewhere explaining that hexyl is "Showing 0 bytes of 58321".

sharkdp commented 3 years ago

I'm honestly just not a fan of printing an empty grid without providing an explanation for it, though

There is also --border=none which would print nothing at all. But okay, I see your point. Let's make it a hard error in that case and disallow anything but positive N.

eth-p commented 3 years ago

Another point that I just came across was the error description for using --length=-1:

negative offset specified, but only positive offsets (counts) are accepted in this context

ErichDonGubler commented 3 years ago

Disclaimer: I'm interested in resolving this. Right now, I'm just going to triage things, but I hope to have solution after I finish researching and put in my vote for my elections as a US resident.

I'm basically going to summarize what's said above, in addition to stating my opinion here (for what it's worth). Sorry if this seems redundant. :)

Thoughts?

sharkdp commented 3 years ago

But I'm not sure if it's worth breaking people at this point

I am always worried about breaking backwards-compatibility, but it seems rather unlikely to me that someone relies on this as a feature.

with a large number of users (which I don't know is applicable here -- @sharkdp?)

Always a very hard question to answer. But judging from these statistics...

.. and from the fact that there are official packages for a number of package managers, I would guess that there are a few thousand users.

Elaborating on @sharkdp's : Some scripts or applications may find it easier (or even make sense) to fall back to generating a 0, rather than special-casing logic to determine whether to call hexyl or emit its own diagnostic.

:+1:

A UI where showing empty content makes perfect sense. For example, a borderless display for a file preview pane like that found in ranger. This is a case where --border=none could actually make a lot of sense, because these sorts of applications/users may prefer that border style in their workflow.

Excellent point.

Thoughts?

I suggest that we further support --length=0 without printing any warning or error. Negative lengths are not supported (and should not be).

I'm going to close the ticket, unless there are objections.