[command] add show command and make tail/head string friendly - #163
[command] add show command and make tail/head string friendly#163rmannibucau wants to merge 7 commits into
Conversation
gunnarmorling
left a comment
There was a problem hiding this comment.
Thanks a lot, @rmannibucau! A few comments and suggestions inline.
| CommandSpec spec; | ||
| @CommandLine.Option(names = {"-s", "--bytes-as-string"}, defaultValue = "false", description = "Render binaries as string.") | ||
| boolean bytesAsString; | ||
| @CommandLine.Option(names = {"-ss", "--sample-size"}, defaultValue = "10", description = "Max number of line used to autoadjust the column width.") |
There was a problem hiding this comment.
Do we truly need that, can't we not just use all rows. I want to minimize options as much as possible.
There was a problem hiding this comment.
use all rows -> quite unlikely until you accept to assign too much gigs to the JVM IMHO, I like to keep it light in memory requirement (streaming), default doesn't require anything so should be ok, it is just to read a few line to try to adjust a bit more (ex: "user agent" vs the actual value which is super wide)
| import java.util.stream.IntStream; | ||
|
|
||
| // note: align text left since it is how people do read in english | ||
| public class StreamedTable { |
There was a problem hiding this comment.
What does "streamed" mean?
There was a problem hiding this comment.
streaming, no materialization in memory of the full table
| +----+----------------+-----------------+ | ||
| | id | prefix_strings | varying_strings | | ||
| +----+----------------+-----------------+ | ||
| | 1 | apple | hello | | ||
| +----+----------------+-----------------+ | ||
| | 2 | application | world | | ||
| +----+----------------+-----------------+ | ||
| | 3 | apply | wonderful | | ||
| +----+----------------+-----------------+ | ||
| | 4 | banana | wonder | | ||
| +----+----------------+-----------------+ | ||
| | 5 | bandana | wander | | ||
| +----+----------------+-----------------+ | ||
| | 6 | band | wandering | | ||
| +----+----------------+-----------------+ | ||
| | 7 | bandwidth | test | | ||
| +----+----------------+-----------------+ | ||
| | 8 | ban | testing | | ||
| +----+----------------+-----------------+"""); |
There was a problem hiding this comment.
Can we have line delimiters only after the header row, it reduces the space needed.
| +----+----------------+-----------------+ | |
| | id | prefix_strings | varying_strings | | |
| +----+----------------+-----------------+ | |
| | 1 | apple | hello | | |
| +----+----------------+-----------------+ | |
| | 2 | application | world | | |
| +----+----------------+-----------------+ | |
| | 3 | apply | wonderful | | |
| +----+----------------+-----------------+ | |
| | 4 | banana | wonder | | |
| +----+----------------+-----------------+ | |
| | 5 | bandana | wander | | |
| +----+----------------+-----------------+ | |
| | 6 | band | wandering | | |
| +----+----------------+-----------------+ | |
| | 7 | bandwidth | test | | |
| +----+----------------+-----------------+ | |
| | 8 | ban | testing | | |
| +----+----------------+-----------------+"""); | |
| +----+----------------+-----------------+ | |
| | id | prefix_strings | varying_strings | | |
| +----+----------------+-----------------+ | |
| | 1 | apple | hello | | |
| | 2 | application | world | | |
| | 3 | apply | wonderful | | |
| | 4 | banana | wonder | | |
| | 5 | bandana | wander | | |
| | 6 | band | wandering | | |
| | 7 | bandwidth | test | | |
| | 8 | ban | testing | | |
| +----+----------------+-----------------+"""); |
There was a problem hiding this comment.
did you try with yellow taxi or a table which is way wider? you have no idea on which line you are so yes it is doable but something more fancy can be desired no?
There was a problem hiding this comment.
adding a boolean to enable this mode for now, kept the delimiter by default but we can swtch it later on
ultimately we can see to reuse https://tomitribe.github.io/crest/tables/border-styles/ (extracting this part in a reusable lib)
I can envision a ~/.hardwoodrc for this kind of thing
| default Iterator<RowReader> toIterator() { | ||
| return new Iterator<>() { | ||
| @Override | ||
| public boolean hasNext() { | ||
| return RowReader.this.hasNext(); | ||
| } | ||
|
|
||
| @Override | ||
| public RowReader next() { | ||
| RowReader.this.next(); | ||
| return RowReader.this; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /// Convenient method to convert this to a Stream. | ||
| default Stream<RowReader> toStream() { | ||
| return StreamSupport.stream(Spliterators.spliteratorUnknownSize(toIterator(), Spliterator.IMMUTABLE), false); | ||
| } |
There was a problem hiding this comment.
Can we have these as internal methods where you need them for now. I want to keep the public API as minimal as possible.
There was a problem hiding this comment.
what about making the row reader an actual Iterator, it is weird it is 95% of it only? will drop it for now
64b6323 to
dca4282
Compare
|
@gunnarmorling addressed comments where it was possible, main pending point will be about the delimiter for tables but my sample files are messed up (one row takes like 4 lines on the screen) without and it is hard to know where I am without the delimiter so I'm mixed about this one |
|
reversed the table style but kept the toggle since it is what makes it usable for now - we might envision to replace it by an empty line or another separator but when data are very wide and needs to not be truncated an option is needed |
|
Could you rebase this one to latest main, now that #144 has been resolved? Thanks a lot. |
8cbc7f4 to
dcb3aa6
Compare
|
@gunnarmorling rebased |
|
Thanks a lot, @rmannibucau! I've rebased this once more and done a range of follow-up changes via #216, which is based on your work. It expands help and docs, introduces the "ALL" sentinel value for the -n flag, adds test for --transpose, and adds projection support. Gonna close this PR in favor of #216. I've also logged #214 as a follow-up. |
No description provided.