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

Make cli sql output prettier. #2237

Merged
merged 3 commits into from
Aug 24, 2015
Merged

Make cli sql output prettier. #2237

merged 3 commits into from
Aug 24, 2015

Conversation

mberhault
Copy link
Contributor

Use github.com/olekukonko/tablewriter rather than tabwriter,
it does tables just the way we want them.

Before:

> select * from system.namespace
parentIDname    id
0 system    1
1 descriptor  3
1 namespace 2

After:

> select * from system.namespace
+----------+------------+----+
| PARENTID |    NAME    | ID |
+----------+------------+----+
| 0        | system     | 1  |
| 1        | descriptor | 3  |
| 1        | namespace  | 2  |
+----------+------------+----+

Use github.com/olekukonko/tablewriter rather than tabwriter,
it does tables just the way we want them.

Before:
```sql
> select * from system.namespace
parentIDname	id
0 system    1
1 descriptor  3
1 namespace 2
```

After:
```sql
> select * from system.namespace
+----------+------------+----+
| PARENTID |    NAME    | ID |
+----------+------------+----+
| 0        | system     | 1  |
| 1        | descriptor | 3  |
| 1        | namespace  | 2  |
+----------+------------+----+
```
@BramGruneir
Copy link
Member

This looks great. But can you add a test to make sure the output matches what we expect?

@tamird
Copy link
Contributor

tamird commented Aug 24, 2015

LGTM. 👎 on the test because that's testing a dependency.

@petermattis
Copy link
Collaborator

LGTM. Now I feel like we're really implementing SQL.

@BramGruneir
Copy link
Member

Yes, we are testing a dependency, which is why it should be simple, but we should make sure it doesn't change out from under us.

@mberhault
Copy link
Contributor Author

@BramGruneir: ditto. we really need tests for all cli stuff (I've had #2008 against me for a while). I may do basic table stuff in there, but I'd rather avoid that entirely. As for changes, we control the revision.

@BramGruneir
Copy link
Member

As long as we'll have some tests on the output somewhere, that works for me.

@mrtracy
Copy link
Contributor

mrtracy commented Aug 24, 2015

I agree with bram, it's going to be difficult to update that dependency with confidence unless we have some sort of test here.

LGTM otherwise.

@tamird
Copy link
Contributor

tamird commented Aug 24, 2015

@mberhault I lied to you! It's make storedeps

mberhault added a commit that referenced this pull request Aug 24, 2015
@mberhault mberhault merged commit 3151c26 into master Aug 24, 2015
@mberhault mberhault deleted the marc/prettify_cli_sql branch August 24, 2015 21:36
@mberhault
Copy link
Contributor Author

Oops. forgot to squash. Oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants