cli: adds \d, \dt, \du, \l metacommands to sql shell

[kenliu]

Also tweaked shell welcome message to mention “\q” instead of “CTRL + D” (doesn’t work on Windows, and isn’t primary command)

Release note: added \d, \dt, \du, \l metacommands to cli shell

[cockroach-teamcity]

This change is 

[kenliu]

lint errors, running again locally

[jordanlewis]

Generally ! A couple of small comments within.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @kenliu)

---

pkg/cli/sql.go, line 48 at r1 (raw file):

# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.

Maybe To exit, CTRL + D or type \q.? (ctrl+d still works, after all.)

---

pkg/cli/sql.go, line 1040 at r1 (raw file):

	case `\d`:
		if len(cmd) > 1 {

Seems like we might want to make this == 1? Otherwise, you could type \d foo bar and it would give you columns from foo and ignore the second parameter completely.

[kenliu]

@jordanlewis I got this lint error, not sure how to resolve pkg/cli/sql.go:1043:10: if block ends with a return statement, so drop this else and outdent its block

[jordanlewis]

It's saying that the else block is irrelevant here:

	case `\d`:
		if len(cmd) > 1 {
			c.concatLines = `SHOW COLUMNS FROM ` + cmd[1]
			return cliRunStatement
		} else {
			return c.invalidSyntax(errState, `%s. Try \? for help.`, c.lastInputLine)
		}

It wants it to become:

	case `\d`:
		if len(cmd) > 1 {
			c.concatLines = `SHOW COLUMNS FROM ` + cmd[1]
			return cliRunStatement
		}
		return c.invalidSyntax(errState, `%s. Try \? for help.`, c.lastInputLine)

[jordanlewis]

(Or vice versa, which IMO might look cleaner, but doesn't really matter)

[kenliu]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @knz)

---

pkg/cli/sql.go, line 48 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Maybe To exit, CTRL + D or type \q.? (ctrl+d still works, after all.)

It turns out that CTRL-D doesn't work on Windows (I tried it) and our help message shows \q as the primary command to exit. There are many others (and it turns out there are undocumented ones) so I thought it best to show just the one here and if someone wants to look for the others they'll figure it out.

---

pkg/cli/sql.go, line 1040 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Seems like we might want to make this == 1? Otherwise, you could type \d foo bar and it would give you columns from foo and ignore the second parameter completely.

good call

[knz]

LGTM but I stand behind jordan's request on if len(cmd) == 1 for the \d case

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

[kenliu]

addressed comments. added an additional test to catch this case too.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis and @knz)

[kenliu]

also fixed lint error

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis and @knz)

[jordanlewis]

Nice tests, too! - please make sure to squash your commits before merging. (as a side note, avoiding this final squash step is why I like to keep amending PRs rather than pushing small new commits :)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

[kenliu]

makes sense, although isn't it easier for the reviewer to see the delta between the last review and and the set of review fixes? Could always squash at the very end.

[jordanlewis]

makes sense, although isn't it easier for the reviewer to see the delta between the last review and and the set of review fixes? Could always squash at the very end.

This problem is why we use the Reviewable tool, which does a decent job of showing you the delta between the last review and the set of review fixes, even if you amend your commit. (It lets you view a diff between 2 versions of the same commit.)

[kenliu]

ohh I see, makes sense.

[kenliu]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[knz]

@kenliu nit for next time: the release note generation program wants to know the category of the release note, so instead of this

Release note: added \d, \dt, \du, \l metacommands to cli shell

you can do this:

Release note (cli change): added \d, \dt, \du, \l metacommands to cli shell

and it's also good to add some explanatory context for users, to motivate the change, for example:

Release note (cli change): `cockroch sql` and `cockroach demo` now support 
the client-side commands `\d`, `\dt`, `\du`, `\l`, for compatibility with, 
and to ease adoption by, users of `psql`.

[kenliu]

Sounds good, thanks. Is there documentation to explain how the release note process works? I didn't really have any idea if these notes were doing to be user facing or if the docs team was going to edit them further.

[knz]

1. the CONTRIBUTING.md file at the top level mentions release notes contains a (partially broken link) to https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

The particular section on release notes is here: https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages#GitCommitMessages-Releasenotes

2. The commit message template also includes guidance as a comment at the bottom. Does your editor not show this to you?

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Commit message recommendation:
#
#     ---
#     <pkg>: <short description>
#
#     <long description>
#
#     Release note (category): <release note description>
#     ---
#
# Wrap long lines! 72 columns is best.
#
# The release note must be present if your commit has user-facing
# changes. Leave the default above if not.
#
# Categories for release notes:
#     - cli change
#     - sql change
#     - admin ui change
#     - general change (e.g., change of required Go version)
#     - build change (e.g., compatibility with older CPUs)
#     - enterprise change (e.g., change to backup/restore)
#     - backwards-incompatible change
#     - performance improvement
#     - bug fix

[knz]

Note that even though the doc team edits them further, they are very much appreciative of text that gives them enough context to understand what's going on.

[kenliu]

Got it, thanks!

On Fri, Aug 9, 2019 at 11:07 AM kena ***@***.***> wrote: Note that even though the doc teams edits them further, they are very much appreciative of text that gives them enough context to understand what's going on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39141?email_source=notifications&email_token=AAALRAXPOYJK3SDAVD7MC3DQDWB3BA5CNFSM4IHLQTH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD366DOQ#issuecomment-519954874>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAALRAVLIYMJBZ563UQXLHLQDWB3BANCNFSM4IHLQTHQ> .

-- Ken Liu | Engineering Manager *Cockroach Labs* *Help us Make Data Easy!* <http://grnh.se/l4bqjc>