Deep-Symmetry / bytefield-svg

Node module that generates byte field diagrams in SVG format
Eclipse Public License 2.0
122 stars 18 forks source link

Allow boxes-per-row > 16 #33

Closed g-radam closed 1 year ago

g-radam commented 1 year ago

Is your feature request related to a problem? Please describe. The variable "boxes-per-row" cannot be incremented larger than 16, only decreased. When larger than 16, an error is returned: "No item x in vector of length 16", So I assume each row is fixed at a maximum of 16 elements.

The diagrams I am creating would be much better visualised as a single row, with a number of bytes > 16, which cannot be achieved. (Multiple diagrams will be created one below the next, in order to visually show subtle differences in each bytefield diagram).

Describe the solution you'd like Allow more than 16 boxes per row, I assume the heading would require two character: 00, 01 ... 0f, 11, 12, 13. I assume the row offsets values would be handled correctly by default.

Describe alternatives you've considered Accept the limit of 16 and let my diagram to wrap onto a second row at the cost of loosing the subtitle visual differences between diagrams.

Additional context

brunchboy commented 1 year ago

Hello! Please include a full stack trace to help understand things in the future. At a glance, though, I would say the problem is that you forgot to supply an appropriate set of column labels for your use case; the default one only has sixteen entries, so it will fail unless you use sixteen or fewer columns. You can supply your own, though. Please see the column-labels entry in Predefined Values in the documentation.

g-radam commented 1 year ago

Hi There! Sorry yes, I didn't include back trace as it the error seemed to indicate it was an obvious out of bounds check due to the "boxes-per-row" > 16:

(def boxes-per-row 32)

Results in:

Error 500: undefined: 0,
Error: No item 16 in vector of length 16

Regardless, Thank you for clarifying that I could use column-labels as that seemed to have fixed the issue.

With that said though, both the error I had received above and the docs in no way indicated that the "boxes-per-row" was capped at 16 by default unless column-labels was defined with > 16 elements. I assumed that the boxes-per-row labels were just an incrementing value, and could be increased > 16. Maybe the docs should be clarified here, or a better error message thrown other than an out of bounds.

Thanks again!

brunchboy commented 1 year ago

The docs are pretty short, so I hoped people would read them over in their entirety and catch things like that. However, if you can come up with a concise way to make this improvement, a pull request would be welcome. I’m sure you are right—my assumption that people would understand that hexadecimal byte charts (which is what this tool was built to display) are limited to sixteen columns, is not clear to everyone. 😄

brunchboy commented 1 year ago

An alternate approach would be to clarify the error message, so that people didn’t even need to refer to the user guide to understand what happened. Catching this situation and returning a clear error explaining to add more column headers would probably be even more user friendly, if you want to try to come up with a PR that does that.