Make the last col of get-classes and get-plans wider so we're at least 80 cols #2226

duglin · 2018-07-23T15:54:49Z

Signed-off-by: Doug Davis dug@us.ibm.com

MHBauer · 2018-07-23T16:07:17Z

cmd/svcat/output/class.go

@@ -32,16 +32,29 @@ func getClassStatusText(status v1beta1.ClusterServiceClassStatus) string {

 func writeClassListTable(w io.Writer, classes []v1beta1.ClusterServiceClass) {


I feel like there's a table writing library we ought to be using.

there is and we're using it. It just doesn't support this feature

duglin · 2018-07-24T00:30:56Z

ping @carolynvs @jberkhahn

carolynvs

I'm not terribly excited about this as a solution for the description column being too narrow. A couple things that I'd prefer to see:

Modify NewListTable() so that all tables follow this formatting, instead of having to do it one-off for each table.
Provide a function that helps to compute the appropriate size for the last column.
Understand what this will look like for tables that don't have long last columns. Maybe show an example of this for a broker that doesn't do long descriptions and another that does? Just so we can see what the final result will look like for different use cases.

duglin · 2018-07-24T16:21:58Z

I thought about how to do some of that stuff. The problem is that if we try to do it "the right way" it'll involve major changes - like creating a proxy structure for each table to track the width of each column as things are appended and calling the proxy for all table ops instead of the real TableWriter. I can take a stab at that but be prepared for a much larger PR with far more impactful changes. I tried to keep things to a minimum with this.

jberkhahn · 2018-07-24T16:46:49Z

I'm not so sure about implementing it that way. Who says every single table we write is going to want an extra large last column? How do we know the last column is always going to be the large one? I think implementing it specifically for the output types that actually need large columns seems fine.

duglin · 2018-07-24T16:49:21Z

If I did the "right (generic) solution" I was going to make it so that people could tell us which column is the variable/wide one. However, before I head down that path and create a much bigger PR I'd like agreement that we really want the bigger PR and not this more focused one. I'll let you two decide - I can go either way.

carolynvs · 2018-07-24T18:00:06Z

NewListTable is intended to be used for the output of svcat get commands, which is why I suggested it. Not all tables are impacted.

Mostly my objection is that I wouldn't want to ever modify/maintain the code added as it looks right now. It's confusing and I'm hoping that there is a middle ground where at the every least the output functions that want to opt in can do so in a way is consistent and doesn't overwhelm the Write* functions with the logic just to format the width...

Honestly I would rather leave it as-is than what's in here. 🤷‍♀️

duglin · 2018-07-24T18:03:48Z

This new output: https://github.com/kubernetes-incubator/service-catalog/pull/2226/files#diff-345f1d72c94dd1c75d5bf43421aa41e2L1 shows why I really want something like this. The old format made us looks silly.

jberkhahn · 2018-07-24T18:06:17Z

Ah, ok. I think the best solution then would be to write some function that figures this out in a generic manner, and then call out to it from writeClassListTable and writePlanListTable

MHBauer · 2018-07-24T19:25:27Z

I don't see anything convincing in #2226 (comment) . Looks fine to me both before and after. I like the consistency of after and the compactness of before

This needs a test and example with some long lines.

…t 80 cols Signed-off-by: Doug Davis <dug@us.ibm.com>

MHBauer

typos

MHBauer · 2018-07-24T22:08:04Z

cmd/svcat/output/table.go

+const DEFAULT_PAGE_WIDTH = 80
+
+// ListTable is a proxy for tablewriter.Table so we can support variable a
+// width column that tries to full up extract space.


full -> fill
extract -> exact
I think

duglin · 2018-07-25T02:08:38Z

@carolynvs see if you like this version better. If so, I'll squash to kill-off the first variant.

carolynvs

This is exactly what I was hoping for! Just one small change, I think that we can cut down on those middleman methods.

carolynvs · 2018-07-25T20:06:53Z

cmd/svcat/output/class.go

 	t.SetHeader([]string{
 		"Name",
 		"Namespace",
 		"Description",
 	})
+	t.SetVariableColumn(3)


❤️ Thanks, this is exactly what I was hoping for!

carolynvs · 2018-07-25T20:07:10Z

cmd/svcat/output/table.go

@@ -22,12 +22,124 @@ import (
 	"github.com/olekukonko/tablewriter"
 )

+// DefaultPageWidth is the page (screen) width to use when we need to twiddle


I am pro "twiddling" 👍

carolynvs · 2018-07-25T20:09:33Z

cmd/svcat/output/table.go

+// value of our special column first, call SetColMinWidth and then add the
+// headers/rows.
+type ListTable struct {
+	table *tablewriter.Table


If you make table an embedded field field instead of a named field, i.e. change this line to *tablewriter.Table without the name, then you don't need to implement the passthrough functions, like SetBorder.

thought about that, but I'm not sure how to do super.Append() when I have my own Append() because I need to override it.

carolynvs · 2018-07-26T14:03:50Z

@duglin I pushed a commit to your branch that shows what I was suggesting.

duglin · 2018-07-26T14:14:06Z

cmd/svcat/output/table.go

@@ -37,26 +37,19 @@ const DefaultPageWidth = 80
 // value of our special column first, call SetColMinWidth and then add the
 // headers/rows.
 type ListTable struct {
-	table *tablewriter.Table
+	*tablewriter.Table


Interesting, I forgot you could ref it by "Table" later on like that. This is why I still feel like such a newbie w.r.t. golang - thanks!

Using embedded fields to satisfy an interface is a really nice feature! 👍

Sorry about pushing to your fork! Next time I send a gist or patch.

No worries, just a "you dunce, just refer it to by Table" would have gotten the point across ;-)

carolynvs

/lgtm

jeremyrickard · 2018-07-30T16:54:23Z

Looks like you fixed Morgan's comment, LGTM.

MHBauer · 2018-08-01T16:56:57Z

cmd/svcat/output/table.go

@@ -22,12 +22,118 @@ import (
 	"github.com/olekukonko/tablewriter"
 )

+// DefaultPageWidth is the page (screen) width to use when we need to twiddle
+// the width of some table columns for better viewing. For now assume its only


its -> it's

MHBauer · 2018-08-01T16:59:13Z

cmd/svcat/output/table.go

+	lt.Table.SetHeader(keys)
+}
+
+// Append will look at each column in the row to see if its longer than any


its -> it's
or
it is

MHBauer · 2018-08-01T17:00:28Z

cmd/svcat/output/table.go

+
+		// If this column doesn't push us past the page width then just
+		// set it to the max cell width so Render() doesn't chop it on us.
+		// Otherwise, chop it so we don't go page the page width.


past the page width

Signed-off-by: Doug Davis <dug@us.ibm.com>

MHBauer · 2018-08-01T18:47:10Z

/lgtm
/approve

k8s-ci-robot · 2018-08-01T18:47:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MHBauer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/svcat/OWNERS~~ [MHBauer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

typos addressed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 23, 2018

k8s-ci-robot requested review from eriknelson and n3wscott July 23, 2018 15:55

MHBauer reviewed Jul 23, 2018

View reviewed changes

duglin force-pushed the widerSvcat branch 2 times, most recently from 8fc428f to 9e70431 Compare July 23, 2018 16:28

duglin force-pushed the widerSvcat branch 2 times, most recently from 5223376 to 32e637c Compare July 24, 2018 15:18

carolynvs suggested changes Jul 24, 2018

View reviewed changes

duglin force-pushed the widerSvcat branch from 32e637c to 44562dc Compare July 24, 2018 16:35

duglin force-pushed the widerSvcat branch from 44562dc to fe053ee Compare July 24, 2018 17:43

Make the last col of get-classes and get-plans wider so we're at leas…

b9a47ba

…t 80 cols Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the widerSvcat branch from fe053ee to 4a5c773 Compare July 24, 2018 22:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2018

MHBauer previously requested changes Jul 24, 2018

View reviewed changes

duglin force-pushed the widerSvcat branch 3 times, most recently from cd1940b to 193c22e Compare July 25, 2018 01:41

carolynvs suggested changes Jul 25, 2018

View reviewed changes

duglin force-pushed the widerSvcat branch 2 times, most recently from 53becd4 to c22d950 Compare July 26, 2018 00:22

duglin commented Jul 26, 2018

View reviewed changes

carolynvs approved these changes Jul 26, 2018

View reviewed changes

k8s-ci-robot assigned carolynvs Jul 26, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2018

carolynvs added the LGTM1 label Jul 26, 2018

jeremyrickard added the LGTM2 label Jul 30, 2018

MHBauer reviewed Aug 1, 2018

View reviewed changes

duglin force-pushed the widerSvcat branch from e86f22e to 26d0cbb Compare August 1, 2018 16:59

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2018

MHBauer reviewed Aug 1, 2018

View reviewed changes

make it generic

70220a0

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the widerSvcat branch from 26d0cbb to 70220a0 Compare August 1, 2018 18:39

k8s-ci-robot assigned MHBauer Aug 1, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2018

k8s-ci-robot merged commit ac6df03 into kubernetes-retired:master Aug 1, 2018

duglin deleted the widerSvcat branch August 2, 2018 20:08

MHBauer mentioned this pull request Aug 2, 2018

v0.1.28 Release #2256

Merged

cblecker unassigned MHBauer Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the last col of get-classes and get-plans wider so we're at least 80 cols #2226

Make the last col of get-classes and get-plans wider so we're at least 80 cols #2226

duglin commented Jul 23, 2018

MHBauer Jul 23, 2018

duglin Jul 23, 2018

duglin commented Jul 24, 2018

carolynvs left a comment

duglin commented Jul 24, 2018

jberkhahn commented Jul 24, 2018

duglin commented Jul 24, 2018

carolynvs commented Jul 24, 2018 •

edited

Loading

duglin commented Jul 24, 2018

jberkhahn commented Jul 24, 2018

MHBauer commented Jul 24, 2018

MHBauer left a comment

MHBauer Jul 24, 2018

duglin Jul 24, 2018

duglin commented Jul 25, 2018

carolynvs left a comment

carolynvs Jul 25, 2018

carolynvs Jul 25, 2018

carolynvs Jul 25, 2018

duglin Jul 25, 2018

carolynvs commented Jul 26, 2018

duglin Jul 26, 2018 •

edited

Loading

carolynvs Jul 26, 2018

duglin Jul 26, 2018

carolynvs left a comment

jeremyrickard commented Jul 30, 2018

MHBauer Aug 1, 2018

MHBauer Aug 1, 2018

MHBauer Aug 1, 2018

MHBauer commented Aug 1, 2018

k8s-ci-robot commented Aug 1, 2018

		@@ -32,16 +32,29 @@ func getClassStatusText(status v1beta1.ClusterServiceClassStatus) string {

		func writeClassListTable(w io.Writer, classes []v1beta1.ClusterServiceClass) {

Make the last col of get-classes and get-plans wider so we're at least 80 cols #2226

Make the last col of get-classes and get-plans wider so we're at least 80 cols #2226

Conversation

duglin commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Jul 24, 2018

carolynvs left a comment

Choose a reason for hiding this comment

duglin commented Jul 24, 2018

jberkhahn commented Jul 24, 2018

duglin commented Jul 24, 2018

carolynvs commented Jul 24, 2018 • edited Loading

duglin commented Jul 24, 2018

jberkhahn commented Jul 24, 2018

MHBauer commented Jul 24, 2018

MHBauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Jul 25, 2018

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs commented Jul 26, 2018

duglin Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

jeremyrickard commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Aug 1, 2018

k8s-ci-robot commented Aug 1, 2018

carolynvs commented Jul 24, 2018 •

edited

Loading

duglin Jul 26, 2018 •

edited

Loading