Use text/tabwriter to align human output #317

sehqlr · 2016-09-26T21:40:03Z

This commit adds the text/tabwriter package to the human prettyprinter
package, and updates test case inputs. This aligns values based on a new
tab inserted to the right of the colon.

Fixes #244

stevendborrelli · 2016-09-27T00:33:45Z

output looks good on some:

root/file.directory.deeper:
        Messages:
        Has Changes: yes
        Changes:
                deeper:       "<absent>" => "<present>"
                deeper/a:     "<absent>" => "<present>"
                deeper/a/b:   "<absent>" => "<present>"
                deeper/a/b/c: "<absent>" => "<present>"

but for docker it is a little ragged:

root/docker.container.nginx:
        Messages:
        Has Changes: yes
        Changes:
                binds:             "" => ""
                command:           "nginx -g daemon off;" => "nginx -g daemon off;"
                dns:               "8.8.8.8, 8.8.4.4" => "8.8.8.8, 8.8.4.4"
                entrypoint:        "" => ""
                env:               "FOO=BAR" => "FOO=BAR"
                expose:            "443/tcp, 80/tcp, 8080/tcp" => "443/tcp, 80/tcp, 8080/tcp"
                image:             "nginx:1.10-alpine" => "nginx:1.10-alpine"
                links:             "" => ""
                name:              "nginx-server" => "nginx-server"
                ports:             "::80/tcp" => "::80/tcp"
                publish_all_ports: "false" => "false"
                status:            "exited" => "running"
                volumes:           "" => ""
                volumes_from:      "" => ""
                working_dir:       "" => ""

Summary: 0 errors, 1 changes

sehqlr · 2016-09-29T15:17:56Z

This is going to need some manual reviews. I set the tabwriter.Debug flag, to make it easier to see what it's doing to the text. We need to decide the parameters to pass into the tabwriter struct, take a look at line 142 in the diff and the tabwriter docs.

BrianHicks · 2016-09-29T15:53:24Z

@stevendborrelli that output actually looks completely fine to me. Are you saying you want it aligned on the arrows as well?

stevendborrelli · 2016-09-29T15:56:45Z

@BrianHicks yes, it should be consistent. One test I had they were lined up, the other one they were not.

BrianHicks · 2016-09-29T16:00:44Z

let me make sure I've got this. Do you want this?

root/docker.container.nginx:
        Messages:
        Has Changes: yes
        Changes:
                binds:             ""                          => ""
                command:           "nginx -g daemon off;"      => "nginx -g daemon off;"
                dns:               "8.8.8.8, 8.8.4.4"          => "8.8.8.8, 8.8.4.4"
                entrypoint:        ""                          => ""
                env:               "FOO=BAR"                   => "FOO=BAR"
                expose:            "443/tcp, 80/tcp, 8080/tcp" => "443/tcp, 80/tcp, 8080/tcp"
                image:             "nginx:1.10-alpine"         => "nginx:1.10-alpine"
                links:             ""                          => ""
                name:              "nginx-server"              => "nginx-server"
                ports:             "::80/tcp"                  => "::80/tcp"
                publish_all_ports: "false"                     => "false"
                status:            "exited"                    => "running"
                volumes:           ""                          => ""
                volumes_from:      ""                          => ""
                working_dir:       ""                          => ""

(I think that's less clear than ragged-right text, personally)

sehqlr · 2016-09-29T16:23:24Z

Here's the output with the Debug |'s inserted:

BrianHicks · 2016-09-29T16:24:47Z

hmm. Maybe we could make the arrow bright and then collapse that space? We're likely to run quite a bit over 80 characters if we align everything.

sehqlr · 2016-09-29T16:29:39Z

We could also make the tabs narrower. Set the tabstop to 4 or 2 instead of 8

BrianHicks · 2016-09-29T16:30:43Z

Doesn’t tabwriter convert to the smallest number of spaces anyway?

On 29 Sep 2016, at 11:29, Sam Hatfield wrote:

We could also make the tabs narrower. Set the tabstop to 4 or 2
instead of 8

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#317 (comment)

sehqlr · 2016-09-29T16:39:41Z

There are controls for adding a number of spaces to each text cell before calculating anything else, as well as a minimum tab width.

BrianHicks · 2016-09-29T16:41:19Z

What about a tab width of 1 and boldening the arrows?

On 29 Sep 2016, at 11:39, Sam Hatfield wrote:

There are controls for adding a number of spaces to each text cell
before calculating anything else, as well as a minimum tab width.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#317 (comment)

sehqlr · 2016-09-29T17:14:11Z

ryane

it looks like tests might need to be updated and the branch is out of date with master. but, if that last screenshot reflects the new output: lgtm

BrianHicks · 2016-10-04T19:26:19Z

prettyprinters/human/human.go

@@ -173,7 +173,8 @@ func (p *Printer) diff(before, after string) (string, error) {
 	// remember when modifying these that diff is responsible for leading
 	// whitespace
 	if !strings.Contains(strings.TrimSpace(before), "\n") && !strings.Contains(strings.TrimSpace(after), "\n") {
-		return fmt.Sprintf("%q\t=>\t%q", strings.TrimSpace(before), strings.TrimSpace(after)), nil
+		sprintf := []string{"%q\t", "\x1b[1m", "=>", "\x1b[22m", "\t%q"}


why are you doing the extra allocation for this?

This is an attempt at readability, but I can make it change it back

That'd be great. We don't need to allocate a list every time 👍

BrianHicks · 2016-10-05T15:22:35Z

prettyprinters/human/human_test.go

@@ -197,7 +197,7 @@ func TestDrawNodeError(t *testing.T) {
 	testDrawNodes(
 		t,
 		Printable{"error": "x"},
-		"root:\n        Error: x\n        Messages:\n        Has Changes: yes\n        Changes:\n                error:   \"\" => \"x\"\n\n",
+		"root:\n Error: x\n Messages:\n Has Changes: yes\n Changes:\n  error: \"\" \x1b[1m=>\x1b[22m \"x\"\n\n",


this should be using the same coloration logic as the rest of the human printer.

sehqlr · 2016-10-05T15:27:20Z

You mean the terminal escape sequence?

On Oct 5, 2016, at 10:22, Brian Hicks [email protected] wrote:

@BrianHicks requested changes on this pull request.

In prettyprinters/human/human_test.go #317 (review):
@@ -197,7 +197,7 @@ func TestDrawNodeError(t *testing.T) {
testDrawNodes(
t,
Printable{"error": "x"},
"root:\n        Error: x\n        Messages:\n        Has Changes: yes\n        Changes:\n                error:   \"\" => \"x\"\n\n",
"root:\n Error: x\n Messages:\n Has Changes: yes\n Changes:\n  error: \"\" \x1b[1m=>\x1b[22m \"x\"\n\n",
this should be using the same coloration logic as the rest of the human printer.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub #317 (review), or mute the thread https://github.com/notifications/unsubscribe-auth/ACnp5EvdbdHqsqvkF2MgZipcVzYd5yiJks5qw8DKgaJpZM4KHBlu.

BrianHicks · 2016-10-05T15:30:26Z

Yep

On 5 Oct 2016, at 10:27, Sam Hatfield wrote:

You mean the terminal escape sequence?
On Oct 5, 2016, at 10:22, Brian Hicks [email protected]
wrote:

@BrianHicks requested changes on this pull request.

In prettyprinters/human/human_test.go
#317 (review):
@@ -197,7 +197,7 @@ func TestDrawNodeError(t *testing.T) {
testDrawNodes(
t,
Printable{"error": "x"},
   "root:\n        Error: x\n        Messages:\n        Has Changes: 
yes\n Changes:\n error: "" => "x"\n\n",
   "root:\n Error: x\n Messages:\n Has Changes: yes\n Changes:\n  
error: "" \x1b[1m=>\x1b[22m "x"\n\n",
this should be using the same coloration logic as the rest of the
human printer.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#317 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACnp5EvdbdHqsqvkF2MgZipcVzYd5yiJks5qw8DKgaJpZM4KHBlu.
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#317 (comment)

sehqlr · 2016-10-05T17:23:49Z

This approach bolds the whole diff, not just the arrow. The bold function should stay as is, because it's useful beyond this case, but if we only want to bold the arrow, I have an idea for a solution.

BrianHicks · 2016-10-05T17:24:29Z

yeah, it should only bold the error. Just add it as another field in the Sprintf call :)

This commit adds the text/tabwriter package to the human prettyprinter package, and updates test case inputs. This aligns values based on a new tab inserted to the right of the colon. Fixes #244

Previous to this commit, I had terminal escape chars embedded in the diff function. With this, I moved that functionality to it's own definition in the prettyprinter's func map. This approach actually bolds more than the arrow, so it may have to be adjusted.

sehqlr · 2016-10-07T19:33:24Z

@BrianHicks @ryane This has been rebased and finished up. Could y'all double check my work and merge, plz?

…values Use text/tabwriter to align human output

sehqlr added the enhancement label Sep 26, 2016

sehqlr added this to the 0.3.0 milestone Sep 26, 2016

sehqlr self-assigned this Sep 26, 2016

ryane mentioned this pull request Sep 30, 2016

Improvement in difference engine #325

Open

ryane requested changes Oct 4, 2016

View reviewed changes

BrianHicks suggested changes Oct 4, 2016

View reviewed changes

BrianHicks suggested changes Oct 5, 2016

View reviewed changes

sehqlr added 6 commits October 7, 2016 14:30

Use text/tabwriter to align human output

8693949

This commit adds the text/tabwriter package to the human prettyprinter package, and updates test case inputs. This aligns values based on a new tab inserted to the right of the colon. Fixes #244

Add tab chars to diff, set Debug flag for review

9f80bd4

Add minor edits to parameters for tabwriter

17504e2

FIX prettyprinter/human/human_test cases

daf98a3

FIX The arrow bolds properly

42c438a

sehqlr force-pushed the feature/align-prettyprinted-values branch from ea503d1 to 42c438a Compare October 7, 2016 19:30

BrianHicks approved these changes Oct 7, 2016

View reviewed changes

BrianHicks merged commit 215933a into master Oct 7, 2016

BrianHicks deleted the feature/align-prettyprinted-values branch October 7, 2016 19:42

BrianHicks added a commit that referenced this pull request Dec 22, 2016

Merge pull request #317 from asteris-llc/feature/align-prettyprinted-…

7189bf1

…values Use text/tabwriter to align human output

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use text/tabwriter to align human output #317

Use text/tabwriter to align human output #317

sehqlr commented Sep 26, 2016

stevendborrelli commented Sep 27, 2016 •

edited

Loading

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

stevendborrelli commented Sep 29, 2016

BrianHicks commented Sep 29, 2016 •

edited

Loading

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

ryane left a comment

BrianHicks Oct 4, 2016

sehqlr Oct 4, 2016

BrianHicks Oct 5, 2016

BrianHicks Oct 5, 2016 •

edited

Loading

sehqlr commented Oct 5, 2016

BrianHicks commented Oct 5, 2016

sehqlr commented Oct 5, 2016

BrianHicks commented Oct 5, 2016

sehqlr commented Oct 7, 2016

Use text/tabwriter to align human output #317

Use text/tabwriter to align human output #317

Conversation

sehqlr commented Sep 26, 2016

stevendborrelli commented Sep 27, 2016 • edited Loading

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

stevendborrelli commented Sep 29, 2016

BrianHicks commented Sep 29, 2016 • edited Loading

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

BrianHicks commented Sep 29, 2016

sehqlr commented Sep 29, 2016

ryane left a comment

Choose a reason for hiding this comment

BrianHicks Oct 4, 2016

Choose a reason for hiding this comment

sehqlr Oct 4, 2016

Choose a reason for hiding this comment

BrianHicks Oct 5, 2016

Choose a reason for hiding this comment

BrianHicks Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

sehqlr commented Oct 5, 2016

BrianHicks commented Oct 5, 2016

sehqlr commented Oct 5, 2016

BrianHicks commented Oct 5, 2016

sehqlr commented Oct 7, 2016

stevendborrelli commented Sep 27, 2016 •

edited

Loading

BrianHicks commented Sep 29, 2016 •

edited

Loading

BrianHicks Oct 5, 2016 •

edited

Loading