Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

buffer status output - fixes #532 #535

Merged
merged 3 commits into from
May 11, 2017
Merged

buffer status output - fixes #532 #535

merged 3 commits into from
May 11, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented May 9, 2017

This is a simple fix for the regression #532 introduced in #525.

@sdboyer Is this sufficient, even if only as a temporary fix? Or is incremental output desired/necessary?
Edit: Not necessary, since tabwriter might not flush until explicitly told to at the end.

@jmank88 jmank88 closed this May 9, 2017
@jmank88 jmank88 reopened this May 9, 2017
@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

Edit: Not necessary, since tabwriter might not flush until explicitly told to at the end.

Definitely - flushing is required, that's the only mode in which the tabwriter is expected to operate (see bottom of docs). The problem, I suspect, is that the underlying log.*Logger takes each discrete Print() call (which the tabwriter seems to do on each field+its tabs) and inject a newline.

Seems like we should have some kind of basic test for this, at least for e.g. the JSON output...? Like, if we had had a test to check that the marshaled JSON output was actually valid JSON, would that have caught this problem in #525?

@jmank88
Copy link
Collaborator Author

jmank88 commented May 10, 2017

Like, if we had had a test to check that the marshaled JSON output was actually valid JSON, would that have caught this problem in #525?

Yeah, tests on the outputs of dep status or just the outputters would have caught it.

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

Can we add a quick test, at least for the JSON output, with a note that it's a regression test for this problem? With dot or the tabwriter output it's harder to verify, but with JSON at least we can verify that our io.Writer has created a valid JSON bytestream by immediately trying to unmarshal the marshaled data.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 10, 2017

Added a test to verify table format output, by extending the integration tests to optionally verify stdout. This lays a foundation to trivially drop in json, dot formats, or any other commands with deterministic output, but I started minimal for now. Let me know if I should just clone this status test and tweak it to cover json and dot as well, or if we should go in another direction.

@sdboyer
Copy link
Member

sdboyer commented May 10, 2017

hah, no, this seems great - more than i was hoping for! 👍 to replicating for dot and JSON output

@sdboyer
Copy link
Member

sdboyer commented May 11, 2017

quick rebase and this is good to go

@sdboyer
Copy link
Member

sdboyer commented May 11, 2017

in we go - thanks!

@sdboyer sdboyer merged commit 070b761 into golang:master May 11, 2017
@jmank88 jmank88 deleted the status_table branch May 12, 2017 12:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants