Add headers to all tables

[heaths]

Resolves #8090

[heaths]

/cc @samcoe

[williammartin]

Adding @samcoe as a reviewer since this has a far larger breadth than I am comfortable giving a stamp for. I'll do a review regardless to ensure I'm learning though.

[williammartin]

Thanks a lot for taking on this task @heaths. I especially appreciate being able to easily look through all the test updates.

I only have one real concern about this and that's the runtime error. I think it makes sense to try and enforce headers going forward but I think I'd rather lean on a compile time enforcement where possible. I don't have much experience with the table printing, so I hold this opinion lightly.

I created a commit that requires maintainers on construction of the table to provide headers (or explicitly opt out in some cases, as you have allowed for in your PR). It's pretty gross since it had to touch every tableprinter instantiation but you should get the idea pretty quickly by looking for tableprinter.WithHeaders and tableprinter.NoHeader. Because go has nil there's no way to prevent nil being passed but at least you really, really have to be intentional about doing that.

In one case I also moved some code around to see whether enforcement at instantiation time had any effects on pushing conditionals "up the stack". Jury is out. It's not that important.

In some other cases this forced instantiation of the tableprinter to be next to its usage point, since it could no longer be injected without headers. I consider this a positive. It makes it clear that nothing else is happening with the table printer which is easier to hold in the head.

Let me know your thoughts, cheers.

[heaths]

That's still optional, though. The idea is to force it unless NoHeaders was passed, which should require additional scrutiny in subsequent PRs. I added that late because in a couple places like gh status headers seemed like a worse UX. As for baking headers into the WithOptions pattern, I think that's fine and can make that change.

If there was an easy way to add linters in a repo, I'd be all for it. The hope here was that it'd fail tests, since tests should be added for new code.

[heaths]

Maybe instead, headers could be required and have a new constructor that creates it without headers - without using the WithOptions pattern.

[williammartin]

Maybe instead, headers could be required and have a new constructor that creates it without headers - without using the WithOptions pattern.

I think you're actually kind of describing the same goal with this? I'm not using the variadic form of the functional options pattern so it's not possible to opt out unintentionally. tableprinter.New(ios) will produce a compile error. The tradeoff is that in my commit tableprinter.New(ios, nil) would not result in any runtime error.

I would expect tableprinter.New(ios, nil) and tableprinter.New(ios, tableprinter.NoHeaders) to both result in additional scrutiny as you said. Perhaps it makes sense to make nil a runtime error that would be caught in the tests while still preserving the additional compile time safety of the rest?

[williammartin]

Initially the signature was tableprinter.New(ios *iostreams.IOStreams , headers []string) but I changed it to the function parameter in order to encourage the readability tableprinter.NoHeaders and to avoid assigning meaning to nil. I wanted nil to be a blaring siren, and I wanted that when people looked around the codebase they would only see examples of WithHeaders and NoHeaders.

The ability to distinguish between nil and tableprinter.NoHeaders is generally also why I did not do split the constructor because tableprinter.NewWithHeaders(ios, nil) is still a case I think it's nice to avoid.

[heaths]

I've been looking through some linters in golangci-lint as well as govet for anything that might work e.g., "require non-nil for this parameter based on a comment or something", but nothing. That would be ideal. I agree runtime error isn't ideal, but figuring that newer code is much better about adding tests (as are the reviewers for requiring it), it would hopefully get caught at test time.

I admit I originally planned to remove that assert - and still will if you all want me to, of course - but as I built atop it, it seemed like a good way to catch issues at test time.

I love Go - I actually prefer it over Rust, when it comes to native languages, for productivity (but not for wasm/wasi) - but there are times I miss some of the expressiveness of other languages e.g., Rust, C#, or even C/C++ with SAL annotations for cases like this.

[williammartin]

I would trade many things for Rust enums

[heaths]

I would trade many things for Rust enums

That is one of the things I like most about Rust, for sure! My day job is mostly C# (though I contribute to several other languages, most notably our Go SDKs) and there's been discussions about adding unions for a couple language versions now. In the Azure SDK, I spearheaded an alternative that mostly meets our needs, but still barely holds a candle to Rust enums.

[heaths]

Talking with a colleague of mine on the Azure SDK for Go team over lunch, he had an idea that might look better. Ultimately, I'd prefer a sentinel value that is more obvious to reviewers - whether they are intimately familiar with the tableprinter package or not - to mean "I explicitly want no headers" and, if needed, scrutinize the decision in the PR. I could take a struct where a zero value should be obvious eg.,

package main

import "fmt"

func main() {
	print(Headers{[]string{"foo", "bar"}})
	print(Headers{})
}

type Headers struct {
	columns []string
}

func print(headers Headers) {
	if len(headers.columns) == 0 {
		fmt.Println("No headers")
		return
	}
	for _, header := range headers.columns {
		fmt.Printf("%s\t", header)
	}
	fmt.Println()
}

A type declaration of []string could still be nil, of course, and seems much less obvious as a sentinel value. Seeing Header{} seems much more clear. Thoughts?

[williammartin]

I've used forbidigo in past to prevent certain patterns (which may or may not work here). If we went down the dual constructor route then we could mark one deprecated with // Deprecated and allow staticcheck to enforce that lint without any further configuration.

I like your proposed approach with Headers{}. It is still possible do do tableprinter.New(ios, Headers{nil}) but that still seems like a nice improvement over tableprinter.New(ios, nil). However...

I have a maybe crazy idea! I know it's usually not idiomatic to return private types from functions because it can constrain the ability to use the type. However, in this case, that's exactly what we're trying to do. Take a gander at this:

type headers struct {
	columns []string
}

var NoHeaders = headers{}

func WithHeaders(columns ...string) headers {
	return headers{columns: columns}
}

func New(ios *iostreams.IOStreams , h headers) ...

Now the only "bad route" is to do tableprinter.New(ios, tableprinter.WithHeaders()). I did play with a signature of func WithHeaders(first string, rest ...string) headers to require at least one column but it was horrible to use for the call sites that construct a slice of headers before passing it to WithHeaders. WithHeaders living alongside NoHeaders makes me pretty confident that we would almost never see tableprinter.WithHeaders() in a PR.

Conclusion

Honestly I think that any of the last few compile (with optional linting help) solutions seem reasonable to me and I'm happy to move forward with any of them and stop dreaming up crazy ideas. I did quite a bit of grunt work in my PR moving all the table.HeaderRow(headers...) calls into constructors so let me know if you'd like me to do some work off that commit for one of the options proposed above.

[heaths]

I like that approach better. It's like the WithOptions pattern, but requires one. If necessary, WithOptions could be added at some future date in a more "proper" fashion using a different type.

This isn't without precedent either. tableprinter.AddField already does this with fieldOption: https://github.com/cli/go-gh/blob/97e972997118b792448133f0144a85e00a8eda74/pkg/tableprinter/table.go#L67

That said, there's still the open question of whether I should update github.com/cli/go-gh to accept headers - optionally, so as not to break extensions after they update, but provide a consistent experience / styling - but for now will keep modifying this source.

[heaths]

To note, I tried to be consistent with headers if a precedent existed that I knew of or could otherwise find.

[heaths]

I made a few options for different header ideas. Just "bold" doesn't really stand out, but here are some other options (the second to last is "black+bd" - I forgot to update the comment):

Since most columns are generally white, I prefer the black+b which does slightly stand out from the gray, which is black+h. All of those are also part of the base VT100 sequences and should render on any terminal these days.

I've updated the PR with black+b but happy to change it to something else you prefer, or even something new. For example, I tried black+bi for inverted and it's not too bad, but 1) feels a bit too boisterous and 2) that last column would need spaces to fill out lest it looks inconsistent:

[heaths]

Underlining them - if we pad the last column (should find a way to do it only for the header or a lot of tests are going to have to be updated!) - actually looks pretty good, too:

The only other issue is untitled headers - for columns that don't really need it or would otherwise be shorter than any suitable column - look a bit odd, but not terrible.

To do the padding right, I think it would be best to move the header support to github.com/cli/go-gh. Padding the first row regardless still makes a lot of tests wrong (in ways that future test authors would likely be confused) and otherwise allocates longer strings than necessary.

[heaths]

Here is how that could work, whether we add underlining to the headers or not:

1. Add header support to tableprinter go-gh#139
2. Use header support in go-gh heaths/cli#32

Sure enough, if I check the output, only the header row is padded, if there is a header row at all. If we go this route, I'd also add tests to ensure that whether returning a ttyTablePrinter or tsvTablePrinter.

/cc @samcoe

EDIT: The thumbnail is hiding the underlines. Click to open the image for a clearer view.

[williammartin]

There's a lot to think about here. I personally don't have a lot of opinion on the colour or bolding of the headers as long as we have some way for it to work with varied background colours.

With regards the underlines, I'm interested to know what screenreaders make of it (I'll have to try it later). With regards to underlines looking strange on columns that have no headers: maybe they should for accessibility reasons anyway? I understand that table output isn't the best for screenreader but I'm sure it's better to have some description of what could be found below rather than nothing.

[heaths]

Using the indexed (16) colors should be good in all cases. Those colors can be remapped, unlike 256 or truecolors, AFAIK. And people will often do that. I remember when Windows added that feature many years back. Comments were like, "about time" and "linux already had that", etc.

Any suggestions for what's next? This touches a lot of files, and I'd love to not have to keep rebasing this PR.

[samcoe]

@heaths Thanks for laying out the various options. My vote is to stick with black+b. I think the underline looks nice but has too many open questions around accessibility and we would need to do a thorough testing before I would be comfortable merging something like that.

I left a couple of comments, I think this is real close.

[samcoe]

What is the reason for not adding "VISIBILITY" header here?

[heaths]

I could - and did originally - but "VISIBILITY" is 10 characters while "public" and "private" are only 6 and 7 characters, respectively. Then column becomes longer. I considered truncation, but arbitrary truncation may not be ideal e.g. would you want to see just "VISIBIL" or, worse, "VISIBI…"? I left the project subcommand headers as-is, but figured in a few places that may no header for obvious values was better than longer-than-necessary columns truncating e.g., description fields more compared to previously.

Alternatively, maybe a new tableprinter function to use a long or predefined short name (default to truncation like is done now) e.g., "VISIBILITY|VIS" such that "VISIBILITY" is shown if it'll fit; otherwise, "VIS" would show.

Or, if you don't care about columns being wider than they were before, I can easily find the places where columns don't have headers and add them in.

[heaths]

With the dependent changes in cli/go-gh#139, I think the underlining really sets it apart (before and after pic):

That said, it's a style not characters so I would think accessibility wouldn't be a problem. When I tried it on Windows Narrator, the underlines nor spaces were announced. If narrators announced colors, any modern terminal would probably be hard to understand with color CLIs, TUIs, etc.

If you still don't want them, should we even take the changes in cli/go-gh#139? Customized control over padding can be useful for other things like centering columns.

PS: A made a couple comments on the other issue because I didn't see notification that you actually did review this. Not sure why. Maybe I dismissed an unread notification by accident. Sorry if any comments came off as impatient as a result.

[heaths]

BTW, I rebased locally on upstream/trunk and getting a ton of linter errors with golangci-lint e.g.,

pkg/cmd/codespace/common.go:188:19: c.DisplayName undefined (type codespace has no field or method DisplayName, but does have displayName) (typecheck)
        displayName := c.DisplayName
                         ^
pkg/cmd/codespace/common.go:191:19: c.Name undefined (type codespace has no field or method Name) (typecheck)
                displayName = c.Name

[samcoe]

@heaths Good news, the team decided to move forward with the underlined headers that you presented. We definitely agree that they look great, and hope you understand our hesitancy with the change. We did more investigation today and felt that the underline experience was no different than without them from an accessibility perspective.

I went ahead and rebased the PR against latest trunk. Additionally, I pulled in the latest go-gh and made some style changes that I thought made sense.

I think there is one last thing I want to get into this and that is protection from truncating the first column and columns displaying URLs. This is something that was automatically handled by our previous tableprinter and I think something that should be added to the one in go-gh. I am going to open a PR in go-gh to add that functionality and then pull that in to this PR once it is shipped.

After this another thing I would like to see cleaned up is the use of non-standard time fields which we use in a couple places throughout the codebase, I think that potentially some of those might be breaking changes so I left them out of this PR.

We appreciate the maximum effort here! I will take this over the finish line. Please let me know if you have any thoughts/questions.

EDIT:
After looking at the URL and first column protection it looks like we have abandoned this when migrating to the new tableprinter. Not sure if that was on purpose or overlooked. I will discuss with the team, but I don't think I am going to hold up this PR for that functionality now. We can always introduce it later before the next release if we feel it is important.

[heaths]

After looking at the URL and first column protection it looks like we have abandoned this when migrating to the new tableprinter. Not sure if that was on purpose or overlooked. I will discuss with the team, but I don't think I am going to hold up this PR for that functionality now. We can always introduce it later before the next release if we feel it is important.

@samcoe You want me to make those changes? I did update the PR to add the URL check back into places where URLs seemed possible (for most fields, it's not e.g., fixed status enums, branch names, repo names, etc.) but doing so automatically might be better, sure. Handling the first column also isn't that hard.

After this another thing I would like to see cleaned up is the use of non-standard time fields which we use in a couple places throughout the codebase, I think that potentially some of those might be breaking changes so I left them out of this PR.

Are you talking about the TimeAgoAbbr styles? I had considered adding an AddTimeAgoAbbrField like the existing AddTimeAgoField (or whatever it's called; I forget off hand) or even pass a bool or func in to change the default behavior, or would you rather make them all consistent. Seems they aren't that different and given its only TTY output it's unlikely breaking - at least, in any way people should ideally be using the CLI in automation.

[heaths]

Nit: @samcoe would you want to call this lightGrayUnderline? It's slightly lighter than gray (depending on terminal color mappings). That's why I decided to originally call the style TableHeader (not complaining you changed it, mind you): it's the obvious style for a table header and doesn't get too specific about the colors.

A few styles are called / include "gray" here and are all slightly different colors.

To note, I choose "white+du" over "black+hu" or "black+bu" because it was slightly different than gray that's used for various columns e.g. dates. Not hugely noticeable - at least not with Windows Terminal's default color mappings - and maybe unnecessary now that we have the underlines.

[samcoe]

That's a good point, will make that change before merging.

[samcoe]

@heaths Sorry that was not clear, I am not asking for or expecting that follow up from you. I just wanted to lay out my vision for future work.

Yeah I am talking about replacing all cases of:

tp.AddField(text.FuzzyAgoAbbr(time.Now(), sometime), tableprinter.WithColor(cs.Gray))

with

tp.AddTimeField(time.Now(), sometime, cs.Gray)

The TTY output change is not breaking, but in the places where text.FuzzyAgoAbbr is being used the non-TTY output has been manually set to some time format. That format may or may not be time.RFC3339 which is used by tp.AddTimeField. So the non-TTY time format might change in some places which may or may not be a big deal.