-
Notifications
You must be signed in to change notification settings - Fork 351
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
Fix newlines in \d output #362
Conversation
drivers/metadata/writer.go
Outdated
@@ -505,6 +504,7 @@ func (w DefaultWriter) describeSequences(sp, tp string, verbose, showSystem bool | |||
params["footer"] = "off" | |||
params["title"] = fmt.Sprintf("Sequence \"%s.%s\"\n", s.Schema, s.Name) | |||
err = tblfmt.EncodeAll(w.w, rows, params) | |||
w.w.Write([]byte("\n")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed only in sequences? we don't add an extra newline after other calls to tblfmt.EncodeAll()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely is incorrect. The EncodeAll
should handle all output. Possibly the EncodeAll
implementation is wrong, but no additional output should be emitted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this will be a bit longer :-)
All other calls to EncodeAll
are executed as part of backslash commands that only print a single table. They all have a single newline at the end of the table. I.e. the new prompt is displayed directly below the table. E.g:
pg:postgres@localhost/postgres=> \dt
List of relations
Schema | Name | Type
--------+------------------------------+----------
public | test_table | table
(1 row)
pg:postgres@localhost/postgres=>
psql behaves differently in this case. It prints one more newline after the "(1 row)".
In the output of \d this becomes an actual issue since it is printing multiple tables. E.g:
table "public.test_table_1"
Name | Type | Nullable | Default
------+---------+----------+---------
id | integer | "YES" |
Indexes:
"test_index" PRIMARY_KEY, index (id)
table "public.test_table_2"
Name | Type | Nullable | Default
------+---------+----------+---------
id | integer | "YES" |
Index "public.test_index_1"
Name | Type
------+---------
id | integer
primary key, index, for table test_table_1
Here we definitely need the additional newline. Otherwise there would be no space between the table summary and the next table. \d is calling describeTableDetails
, describeSequences
and describeIndex
.
In describeTableDetails
the extra newline is created as part of the summary. The summary callback ends in a newline and tblfmt adds another newline (https://github.com/xo/tblfmt/blob/master/encode.go#L580).
In describeIndex
I created the same behavior by adding a newline to the summary.
describeSequences
is calling EncodeAll
without a summary callback, so I manually added the newline here. The only way I see how to create a double newline with EncodeAll
for this case would be to have a summary callback with an empty summary (an empty summary will trigger tblfmt to add a newline). Otherwise it might make sense to add an option to the tblfmt encoder to create an extra newline at the end of the table (with or without summary). I don't think there is such an option currently.
So overall I see two options:
- Continue to create the double newline as part of the summary. If there is no summary an empty summary would need to be created for an extra newline. A newline could also be added to the default summary to fix the first example.
- Add an option to the tblfmt encoder to optionally add a newline at the end of the table.
I think the second option is cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a PR for the required changes to tblfmt. The PR additionally fixes the output of the other \d commands by adding a trailing newline by default.
This PR currently points to the tblfmt branch in my fork. I will revert the changes to go.mod once the tblfmt changes are merged.
TestWriter test now succeeds for pgsql descTable
beb5ac8
to
73b9437
Compare
This fixes the output of
\d *
. There were too many newlines printed at the beginning of the output. And no newlines were printed to separate indices and sequences. E.g.:TestWriter now also succeeds for pgsql descTable
go test ./drivers/ --dbs pgsql --cleanup=false
I restricted the postgres implementation of Tables() to only return tables, views, materialized views and sequences. Technically this would not be necessary since all calls to Tables() now specifically request the types of database objects they require. But still I think it is cleaner since other implementations (mostly information_schema) behave like this as well.
This PR depends on (PR36)[https://github.com/xo/tblfmt/pull/36] in tblfmt.