cli: avoid special line at the end of CSV/TSV output

[knz]

Fixes #18584.

Prior to this patch, the csv/tsv formatters would end a table with a
row count. This is not great for interoperability with external tools
which may expect to be able to redirect the table output to a file and
open that directly in a csv/tsv reader, where the final row count
would be incorrectly interpreted as an extra row.

This patch changes the behavior by omitting the row count altogether
for csv/tsv output.

Release note (cli change): when printing tabular results as CSV or
TSV, no final row count is emitted. This is intended to increase
interoperability with external tools.

[cockroach-teamcity]

This change is 

[jordanlewis]

Oo, can you include an example of the before/after for the end-user in the PR?

[bdarnell]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/cli/flags.go, line 369 at r5 (raw file):

		f := cmd.Flags()
		VarFlag(f, &cliCtx.tableDisplayFormat, cliflags.TableDisplayFormat)
		BoolFlag(f, &cliCtx.displayRowCounts, cliflags.DisplayRowCounts, cliCtx.displayRowCounts)

In my mind the row count is a part of the pretty format and not something that can be toggled on/off independently. This knob and the associated implementation of row counts for every format seem like unnecessary complexity to me.

---

Comments from Reviewable

[knz]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/cli/flags.go, line 369 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

In my mind the row count is a part of the pretty format and not something that can be toggled on/off independently. This knob and the associated implementation of row counts for every format seem like unnecessary complexity to me.

I started making it configurable for csv/tsv (there are legitimate cases where we want a row count for them) and then I wasn't happy with having it configurable form csv/tsv. In particular it became clear that at least the html and records formatters should also have it. But then for orthogonality in docs I figured that it should be the same everywhere so the explanation remains simple. What do you think?

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/cli/flags.go, line 369 at r5 (raw file):

Previously, knz (kena) wrote…

I started making it configurable for csv/tsv (there are legitimate cases where we want a row count for them) and then I wasn't happy with having it configurable form csv/tsv. In particular it became clear that at least the html and records formatters should also have it. But then for orthogonality in docs I figured that it should be the same everywhere so the explanation remains simple. What do you think?

What is that use case? The reason I was thinking it was format-specific was because I couldn't think of any time you'd want anything but CSV/TSV data when using those formats.

---

Comments from Reviewable

[knz]

I have removed all commits but the last and simplified the implementation. PTAL.

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/cli/flags.go, line 369 at r5 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

What is that use case? The reason I was thinking it was format-specific was because I couldn't think of any time you'd want anything but CSV/TSV data when using those formats.

Okay, removed, PTAL.

---

Comments from Reviewable

[bdarnell]

Reviewed 9 of 9 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/cli/format_table.go, line 289 at r7 (raw file):

func (p *csvReporter) doneRows(w io.Writer, seenRows int) error {
	p.csvWriter.Flush()
	// Ensure that there is a separator, in case multiple csv/tsv tables

Even a blank line can make things more complicated for readers. For example, python's csv.reader will return an empty list for the blank line, so reading code needs to explicitly handle it. We shouldn't add a blank line in the common case of a single query. If we want a separator, we should only use it when there are multiple queries. But even then I don't think it's worth the effort - executing multiple queries in a single CLI invocation and using CSV output seems like a very unusual workflow.

---

Comments from Reviewable

[knz]

Removed the empty line as suggested. PTAL

[knz]

TFYR!