Add header support to tableprinter

[heaths]

Related to cli/cli#8090

[samcoe]

@heaths While in the table header PR for gh there was talk about ways to enforce that tables get headers I think in go-gh we should not have the same requirement and rather allow flexibility to the users to decide if they want to follow the same styling as gh.

How I am envisioning this working is that the tableprinter constructor does not change as illustrated by this PR but instead we update the TablePrinter interface to include a method to add headers:

type TablePrinter interface {
	AddField(string, ...fieldOption)
        AddHeaders([]string, ...fieldOption)
	EndRow()
	Render() error
}

What are your thoughts?

[heaths]

That's fine. They main points were to:

1. Handle cases were we need access to privates for calculated e.g., underlines (if we go that route).
2. At least provide address to some consistency for extensions.

The latter makes me wonder if we either:

1. Declare a default style as a public.
2. Just apply the style by default.

The problem is that there is no style support in this module currently, which is why I passed it in.

[samcoe]

Hmm my initial thought was that we might just add in a code comment and or code example showing how to achieve the default look achieved by gh rather than actually adding style specific code inside this package. Keeping the styling decoupled from the printing was one of the goals Mislav was trying to achieve when architecting this package.

[heaths]

That's fine, too. I was mainly going for consistency - at least making it easily possible to be consistent - for extensions.

[heaths]

I made the changes and, IMO, the tables look nice in the CLI (using "white+du"):

There is a caveat, though. By padding the last header column, TTY table assertions need to be padded in the header row, which I suspect many CLI devs won't expect. It's solvable, but by exposing some things publicly that might seem odd in isolation. For example, a separate fieldOption could be defined where the callback took a bool whether the column is the last column or something. The CLI color function could then use that to trim the string if the ColorScheme isn't enabled. I have a few other similar ideas.

The underlines really make it look like a header, and even looks nice with a full background color:

But perhaps it's not worth the maintenance hassle. Thoughts? if we don't try to support underlines or other background styles, there's no reason not to trim the last column header, which is what happens currently.

/cc @williammartin

[heaths]

To note, if you decide the support for full-column styles isn't worth the maintenance hassle or workaround, no change should be needed to this module and the other PR is pretty much done and ready for review.

[samcoe]

@heaths I have a different proposal for supporting full column styles which I think is a bit less clever than the approach you are thinking. What about introducing a WithPadding method function that is very similar to WithTruncate? We could then add a padding function to the text package which would pad to string as necessary. This would allow for the most amount of customization and follow a pattern that we have already laid out. Obviously this could allow our users to create disproportionate looking tables, but I would say that is their business.

[samcoe]

Ok kind of an odd question, but should this be AddHeader? I don't have an opinion, just linguistically wondering what is correct. Are we adding a table header, or are we adding table headers?

[heaths]

I could play around with something like that, which is more or less one idea I was considering. That said, it's not even worth it unless you and the others prefer the underlined table header. Certainly easier - still not bad looking, but then relies entirely on color - to only use a different, IMO dimmer, color for the header.

[heaths]

I suppose one interesting use case for a WithPadding func is to allow people to center columns - header or otherwise.

[heaths]

I went ahead and made the changes. It's not a lot of work, and I still personally believe the TableHeader style I defined in the CLI looks nice, but certainly open to changes, of course. As I mentioned above, if nothing else, WithPadding can be used to center column headers, of which my new test shows an example.

cli/cli#8090 already takes advantage of this, so the PR will fail, of course. But I wanted to show how I'm using it, basing it on ColorScheme.Enabled() (newly exposed) so I can mitigate the right padding on the last column so that all existing tests and - for better maintainability - all new tests don't have to account for it.

[samcoe]

@heaths This is great. I pushed a small commit to extract text.PadRight function that is reusable for other users. I am glad we are exposing the WithPadding functionality as gh is not the only consumer of this package.