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

Print human-readable tables for CLI queries. #1523

Merged
merged 2 commits into from
Feb 25, 2016
Merged

Conversation

enisoc
Copy link
Member

@enisoc enisoc commented Feb 25, 2016

@michael-berlin

The table output is similar to the mysql CLI.

Review on Reviewable

@alainjobart
Copy link
Contributor

LGTM with one minor comment: do you have one end2end test that exercises the non-JSON output?

Approved with PullApprove

@michael-berlin
Copy link
Contributor

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


go/vt/vtctl/query.go, line 501 [r1] (raw file):
logutil.Logger being anonymous makes it more difficult for me to grasp here that "lw.Printf" will call the Printf from "logutil.Logger". I would prefer if you give the member a name.


test/tabletmanager.py, line 109 [r1] (raw file):
Turns out we have a JSON Style Guide :)

https://google.github.io/styleguide/jsoncstyleguide.xml?showone=Property_Name_Format#Property_Name_Format

Property names must be camel-cased, ascii strings.

That means the first letter should be lowercase. Do we want to change it to match that style? You just have to add the json tag to the struct for that?


Comments from the review on Reviewable.io

@enisoc
Copy link
Member Author

enisoc commented Feb 25, 2016

I didn't write a test for human-readable output yet because I didn't want it to be a change-detector test. I'll think about it again and see what makes sense.

@enisoc
Copy link
Member Author

enisoc commented Feb 25, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


go/vt/vtctl/query.go, line 501 [r1] (raw file):
I made the call explicit and added a comment that explains what it's doing.


test/tabletmanager.py, line 109 [r1] (raw file):
At first I did make them lowercase with field tags, but then I noticed the rest of the execute commands on vtctl (VtGate_, VtTablet_) were already returning JSON with capital names. So I made mine consistent with those. If you prefer, we could change all of them to lowercase? That would require adding JSON tags on sqltypes.Result.


Comments from the review on Reviewable.io

@michael-berlin
Copy link
Contributor

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


test/tabletmanager.py, line 109 [r1] (raw file):
Thanks for keeping it consistent :)

If it's not too much work for you, let's make all lowercase? I'll let you decide.


Comments from the review on Reviewable.io

The table output is similar to the `mysql` CLI.
@enisoc
Copy link
Member Author

enisoc commented Feb 25, 2016

Review status: 4 of 16 files reviewed at latest revision, 1 unresolved discussion.


test/tabletmanager.py, line 109 [r1] (raw file):
So it turns out generated protobuf code doesn't follow the JSON style guide either. It uses underscore names rather than camel case:

https://github.com/youtube/vitess/blob/master/go/vt/proto/query/query.pb.go#L263

Since querytypes.Result contains querypb.Fields, we have to follow the protobuf convention to keep it consistent.


Comments from the review on Reviewable.io

@michael-berlin
Copy link
Contributor

LGTM

Thanks for updating the JSON fields!


Reviewed 4 of 4 files at r3, 8 of 8 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

Approved with PullApprove

enisoc added a commit that referenced this pull request Feb 25, 2016
Print human-readable tables for CLI queries.
@enisoc enisoc merged commit 6dc99de into vitessio:master Feb 25, 2016
@enisoc enisoc deleted the cli-tables branch February 25, 2016 23:53
dbussink added a commit that referenced this pull request Jan 30, 2023
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
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.

4 participants