Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: use pretty-formatted tables only with terminals or --pretty. #7268

Merged
merged 1 commit into from Jun 16, 2016

Conversation

knz
Copy link
Contributor

@knz knz commented Jun 16, 2016

Prior to this patch all commands reporting table results would
print them using ASCII art tables (pretty-formatted). This makes
the output difficult to parse in automated scripts etc.

This patch corrects the behavior by using TSV output when the command
is not used from an interactive terminal. The pretty-formatted output
can be forced for non-terminal use using the command-line argument
--pretty.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Jun 16, 2016

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


cli/cli.go, line 66 [r2] (raw file):

// isInteractive indicates whether both stdin and stdout refer to the
// terminal.
var isInteractive bool

you can put the initialization inline here.

var isInteractive = isatty.....


cli/context.go, line 57 [r2] (raw file):

type sqlContext struct {
  // Embed the cli context.
  *cliContext

this should not be a pointer


cli/flags.go, line 447 [r2] (raw file):

      // By default, client commands print their output as
      // pretty-formatted tables on terminals, and TSV when redirected
      // to a file. The user can override with --pretty.  We set the

double space


cli/flags.go, line 519 [r2] (raw file):

      // Automatically print pretty-printed tables on interactive
      // terminals.
      cliCtx.prettyFmt = cliCtx.prettyFmt || isInteractive

this doens't handle the case of a user wanting non-pretty tables on the CLI with --pretty=false.


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Jun 16, 2016

Review status: 5 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


cli/cli.go, line 66 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can put the initialization inline here.

var isInteractive = isatty.....

Done.

cli/context.go, line 57 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this should not be a pointer

I believe it should; otherwise the changes performed to the global cliCtx by the flag logic will not be visible from sqlCtx.

cli/flags.go, line 447 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

double space

Done.

cli/flags.go, line 519 [r2] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this doens't handle the case of a user wanting non-pretty tables on the CLI with --pretty=false.

Done.

Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Jun 16, 2016

:lgtm:


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

Prior to this patch all commands reporting table results would
print them using ASCII art tables (pretty-formatted). This makes
the output difficult to parse in automated scripts etc.

This patch corrects the behavior by using TSV output when the command
is not used from an interactive terminal. The pretty-formatted output
can be forced for non-terminal use using the command-line argument
`--pretty`.
@knz knz merged commit 2ecebf3 into cockroachdb:master Jun 16, 2016
@knz knz deleted the format-cli branch June 16, 2016 13:24
@knz knz mentioned this pull request Sep 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants