Add table and helper template functions

[heaths]

Resolves #3488

[mislav]

This is going along great! Thanks for working on this ❤️

[heaths]

In practical manual testing, I found this doesn't work with pagination because the template is executed for each page. I have a couple ideas how to handle that, but feel like it might warrant a separate issue and that for most cases this PR is good enough and it provides time for any other user feedback. That said, I can tackle it as part of this PR if you'd prefer.

For example, one way is to merge output like built-in commands do, but this might have to be opt-in (perhaps we could scan if "{{table" is included in the template) so there are no backcompat issues if users were expecting a JSON array of results that themselves have a arrays, pageInfo, etc. That array itself couldn't be executed against the template because you'd need a separate {{range .}} at the start to enumerate the top-level JSON array. But by merging results, non-array data would be lost, though perhaps that is only pageInfo in practice so perhaps not of concern.

On a related note, the template is parsed each time it's executed. While that isn't a particular lengthy operation (especially for a short-lived console app), making template execution stateful and parsing once would be more efficient. If/when we solve the multiple template execution issue, that would be a good time (perhaps even necessary time) to refactor that.

[heaths]

I refactored to use TablePrinter and solved the pagination issue while I was at it by making the template stateful and rendering the TablePrinter after all pages were processed. I tested pagination on a variant of https://heaths.dev/tips/2021/04/22/gh-user.html using the simple template {{range .data.repository.assignableUsers.nodes}}{{row (.login | color "green") .name}}{{end}}.

Apart from adding some more test cases, I also did some manual regression testing. gh issue list is unchanged, and with these changes I can achieve the same result with a custom template:

(Admittedly it looks pretty long, but I repeated the row to handle the case without labels and not printing "()".)

[heaths]

I've verified this in a simple terminal installed with xfce on Ubuntu 18.04 as well. Table prints properly both to the terminal as well as to a pipe (less).

[heaths]

After installing the private build to my $PATH and adding a few more useful aliases, I find that sometimes I don't want a column to use a full width when the terminal size supports it - most column data is too spread out to easily tell to which row it belongs.

❔ Do you think it'd be fine to add the truncate function back in if users want to control max column width?

[heaths]

@mislav anything else you'd like me to update? And what do you think about me adding the truncate function back in if people want to shorten columns themselves? In my case, I found an example where a single long name (a bot using marketing names) made a table somewhat difficult to read.

I've been using my private version for all operations (going through the new code paths or not) and everything has been working well.

[heaths]

@mislav, @vilmibm is there anything else you'd like me to change? I rebased on master for a few changes that went out in the recent 1.10 release. I had simplified Exporter.Write rather than pass yet another parameter since the method signature would have to change anyway; but, this way I just pass the whole *IOStream rather than several member fields.

[heaths]

Rebased on trunk for conflict in go.mod.

[heaths]

If it makes things easier to review, I recommend not reviewing the changes since my first iteration. I pulled most of that code out and the diff will be larger and largely unnecessary to review. Reviewing the PR anew will probably be easier.

[mislav]

@heaths Thanks for keeping the PR up to date. I still think the feature is worthwhile. However, I still haven't been able to test out how this works in real-world scenarios and think about whether I would want some API changes before we ship. I will be leaving for a short vacation for a few weeks. If no other engineers pick this up in the meantime, I will be circling back to this as soon as I come back. 🙇

[heaths]

The changes I committed a couple months ago to use the existing table formatter greatly simplified the public API to a single {{row}} declaration, which itself can take any pipeline member. It's simple and open by design.

I have a couple examples of use here which not only replicate the built-in gh issue list command output (not practical, but proof of concept) but implement a gh users %1 alias to find user aliases for matching people, along with printing their statuses if anything is set.

Hopefully these can help with practical examples.

aliases:
    issues: |-
        issue list --json number,title,labels,updatedAt --template '{{range .}}{{if .labels}}{{row (printf "#%v" .number | autocolor "green") .title (pluck "name" .labels | join ", " | printf "(%s)" | autocolor "gray+h") (timeago .updatedAt | printf "about %s" | autocolor "gray+h")}}{{else}}{{row (printf "#%v" .number | autocolor "green") .title "" (timeago .updatedAt | printf "about %s" | autocolor "gray+h")}}{{end}}{{end}}'
    users: |-
        api graphql --paginate
        --template '{{range .data.repository.assignableUsers.nodes}}{{if .status}}{{row (autocolor "green" .login) .name (autocolor "gray+h" .email) (autocolor "yellow" .status.message)}}{{else}}{{row (autocolor "green" .login) .name (autocolor "gray+h" .email) ""}}{{end}}{{end}}'
        -F owner=':owner' -F repo=':repo' -F name='$1' -f query='
        query ($repo: String!, $owner: String!, $name: String!, $endCursor: String) {
          repository(name: $repo, owner: $owner) {
            assignableUsers(first: 100, after: $endCursor, query: $name) {
              nodes {
                login
                name
                email
                status {
                  message
                }
              },
              pageInfo {
                hasNextPage
                endCursor
              }
            }
          }
        }
        '

With the latter one, even if I provide an empty string e.g. '' on the command line, the entire list of repo users are formatted nice and neat like any built-in command.

[heaths]

It's also a quick and simple way to list labels:

 gh api /repos/Azure/azure-sdk-for-net/labels --template '{{range .}}{{row .name .color .description}}{{end}}' --paginate

What'd be really cool to build on top this is a function to take the HTML color code and turn that into appropriately-colored labels. Even without that, {{row}} makes it really easy to format a beautiful table just like built-in commands.

[heaths]

To follow up on my comment suggesting you review this anew, it's actually less code if reviewed anew than the PR was originally since I ended up removing a bunch of code. There are more files touched, but most are 1- or 2-liners to change interface implementations to pass in a different parameter type, which should mitigate having to change those signatures as much in the future and may help increase code coverage since you can now mock all of iostreams.IOStreams.

[heaths]

Resolved conflicts with upstream.

[heaths]

@mislav now that you're back, are you able to take a look at this? The majority of changes to the 22 files in the PR are just a couple lines of refactoring if you review the PR anew and not as a diff from when I added my own table formatter, which I took out to use the one you already had. Given that, none of the file diffs are particular large. The largest appears to be documentation updates I made to the usage text.

[mislav]

Sure I will take a look tomorrow! Thanks for your patience. 🙇

[mislav]

This is all very close! I was at first alarmed at the amount of changes, but then I realized that most of them were because a Template now needs access to the IOStream instance, and for good reason, so all call points needed to be edited. 👍

Apart from mostly nitpicky code comments, my last concern is about what would happen if my Go template also emitted some content outside of the row helper? E.g.

START TABLE
{{range .}}{{row "a" "b"}}{{end}}
END TABLE
some footer content

As it stands right now, it looks like all outputted content will be present early in the document, with the table at the very end. Per example above, "footer content" appearing before the whole table might be counter-intuitive to the caller.

Should we reconsider an explicit helper to render a table? E.g.

START TABLE
{{range .}}{{row "a" "b"}}{{end}}
{{tablerender}}
END TABLE

[heaths]

I'll look into options regarding the table print order. I was able to repro where the table was always last. If something like '{{tablerender}}' (or perhaps '{{tableend}}' would be more idiomatic) were necessary, I would hope making it optional is okay. With out it, the table would render at the end as it does before any changes. With it, it would render immediately.

Any other comments inline.

[heaths]

@mislav when adding some tests for {{endtable}} I found that TablePrinter renders inconsistently depending on the terminal being TTY. The TSV printer will render rows immediately so you don't even need {{endtable}}. Should I make this consistent?

[heaths]

I went with {{endtable}} because, while it does render the table immediately, it also allows subsequent tables with different rows to be used, thus ending the current/preceeding table. I do think it's worth not requiring {{endtable}} per my original design because it's less authoring, and I expect most uses of {{row}} are to format a single table much like your build-in table renders for commands like issue list, pr list, etc.

{{endtable}} also feels more idiomatic and more natural to type considering the built-in template {{end}}.

As an example of using multiple tables, here's an alias I added to my ~/.config/gh/config.yml:

aliases:
    prcomments: |
        pr view $1 --json number,title,reviewDecision,body,assignees,comments --template '{{printf "#%v" .number | autocolor "green"}} {{.title}} ({{autocolor "yellow" .reviewDecision}})

        {{.body}}

        {{row (autocolor "gray+h" "ASSIGNEE") (autocolor "gray+h" "NAME")}}{{range .assignees}}{{row (autocolor "green" .login) .name}}{{end}}{{endtable}}
        {{row (autocolor "gray+h" "COMMENTER") (autocolor "gray+h" "ROLE") (autocolor "gray+h" "COMMENT")}}{{range .comments}}{{row (autocolor "green" .author.login) .authorAssociation .body}}{{end}}'

[mislav]

This looks good! So as it stands right now, endtable is optional because every template implicitly ends with an endtable? I like this behavior because it simplifies table-only templates, but wonder whether the magic-ness of this will come back to bite us. For now, I'm fine with this approach. Thanks for the updates!

Questions for team:

• How do we feel about row/endtable helper names? To drive a point further that they are related, should we call them tablerow/tablerender, even if that's more verbose?
• Should tables be rendered in TSV format for non-tty output streams instead of as a table? This is the default behavior of table-rendering commands such as gh issue list. When rendering from Go templates, I wonder whether this magic-ness would be undesireable. Note that our color helper always outputs color, and to pay respect to TTY status we have the autocolor helper.

[heaths]

@mislav I renamed the functions and rebased on trunk, but realized printing a formatted table to TSV will require passing TtyTablePrinter a maxwidth to calculate column widths. Not only would assuming the terminal width be problematic if no TTY is actually available, but assuming some large value means a table could be practically unreadable. Considering a max int you could have a text column be so long that any other columns that come after them would be unusable (one long row makes the rest of the rows nearly impossible to correlate).

1  One    Some title text                                                                                                                                                                                                 OPEN
2  Two    Really, really long text that just continues to scroll off to the right but just to simulate my point. This should cause a scroll bar that then makes you have to scroll way over just to see the next column.  OPEN
3  Three  Shorter text                                                                                                                                                                                                    CLOSED

Is there an advantage to writing a formatted table to a file?

[heaths]

That said, I do have that change ready as well if that's indeed the direction to go. But would it then make sense to pull back in the truncate function I had originally? If columns could be too long, at least then they could trim them to size with ellipsis as needed, same as the TablePrinter now.

[heaths]

So as not to hold this up further, I've made the always-TTYL change and added the truncate template function. Either or both of these could be reverted, but I think if you still want to always print TTY despite my concern, the truncate function would be a mitigation to potentially long columns.

[mislav]

Not only would assuming the terminal width be problematic if no TTY is actually available, but assuming some large value means a table could be practically unreadable.

In my testing I've found that measuring terminal width even when stdout is not attached to the terminal is possible: https://github.com/cli/cli/pull/4146/files#diff-ae00d644eeb217b62415039b66c26ca1ae36b4ac3411c774995bfb9f38ab24d7

[heaths]

Platform-specific, though. Do you see any problem with just using MaxInt when redirected? If you're directing to a file, you may not want to be constrained by a smaller terminal width when you can put more in a file. This has been one of my major gripes with PowerShell since I started using it as "Monad": redirecting to a file still uses the terminal width, so you have to pipe to Out-String (or at least did for a long time; I think it was fixed at some point in Core).

If columns could be too long, users could use the truncate template function I added back in.

FWIW, my inspiration for this change earlier this year was Docker, which uses tables (but not through a template and cannot be combined with other template functions), and it too emits effectively space-delimited text to a file as to a TTY. So I suppose it's not all that odd. It certainly makes more sense when combined with more tables or headers/footers like in my help topic sample.

[heaths]

Hoping with these changes it's close to being merged. I use table templates quite often and I think people will like them.

That said, through practical use, I found one issue I could either push to this PR or open another if it helps get this PR in sooner. I doubt most people would hit it anyway.

If you output multiple lines of text - like comments - the newlines are rendered, which throws the table off (columns still line up, though):

I propose a change that effectively "truncates" these lines, where the first new line is effectively replaced with "..." and the test dropped. It would still use text.Truncate to make sure that the "\n" -> "..." didn't overflow a column, of course.

The question is where? While I couldn't find any built-in use of the TablePrinter that could yield this problem i.e. only single-line strings were rendered in table views, I would think handling this automatically in TablePrinter.AddField would be the most robust. The cost is at least an extra O(n) per string, though, to get the length of the string. So there's certainly reason to limit this to just when the template tableRow function is called. I'm leaning more toward the latter given the perf, though I doubt in the grand scope of a network-bound app it would be relatively significant.

[mislav]

That's a great catch. I would vote to disallow any literal newline character appearing in a table cell. It would always ruin a table.

How about replacing all \n characters (including other whitespace characters around it) with a single space? Then applying the usual truncating logic to it.

[heaths]

I've got a change almost done. Just adding test cases. So would you want this to always run for TablePrinter.AddField, or just when processing {{tablerow}}. For now I just pass it as the truncateFunc to AddField for {{tablerow}}. Currently there's no case where built-in commands would seemingly need this.

I created a separate text.TruncateColumn because the existing text.Truncate that is used by default for AddField is also used elsewhere. The logic is slightly different. Figured TruncateColumn would hopefully make that nuance and use-case obvious.

One behavioral difference is that I replace "\r" or "\n" with "..." instead of just a space because the text does go on, which is exactly what the ellipsis represents. I don't want to give the impression the first line - if less than maxWidth otherwise - is the entire body. There is more, and I think it's important that's represented just like text.Truncate does today.

[mislav]

Fantastic. Thank you for the hard work!!