Skip to content

fix(cli): print farewell after successful cleanup - #116

Open
dcavalcante wants to merge 2 commits into
balajz:mainfrom
dcavalcante:fix/cli-farewell-after-cleanup
Open

fix(cli): print farewell after successful cleanup#116
dcavalcante wants to merge 2 commits into
balajz:mainfrom
dcavalcante:fix/cli-farewell-after-cleanup

Conversation

@dcavalcante

@dcavalcante dcavalcante commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This is a follow-up to #110 and addresses the cleanup ordering noted in this review comment.

PR #110 correctly removed the farewell from --help and --version, but moved it into RunE, before PersistentPostRunE closes the application, database client, and logger. If cleanup fails, the farewell is printed before the command returns an error.

This change moves the farewell to the end of PersistentPostRunE, so it is printed only after cleanup succeeds.

Because persistent hooks can also run for child commands, the farewell is limited to the root command. This keeps future non-interactive commands, such as pgxcli export, from including the interactive farewell in their output.

Reproduction

Using the default history path:

touch ~/.pgxcli_history.jsonl
chmod a-w ~/.pgxcli_history.jsonl
go run . <connection arguments>

Exit pgxcli with \q.

On the current main, the farewell is printed before the history-save error. With this change, only the error is printed.

Remember to restore the file permissions:

chmod u+w ~/.pgxcli_history.jsonl

Comment thread internal/cli/root.go
},

PersistentPostRunE: func(_ *cobra.Command, _ []string) error {
PersistentPostRunE: func(cmd *cobra.Command, _ []string) error {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Daniel. One more thing I noticed is that when closing resources such as the DB, logger, etc, the current implementation exits the function as soon as an error occurs while closing a resource and prints the close error.

To make it more robust, how about collecting any errors that occur while closing the resources and returning them all at the end, if there are any? In that case, the output would be something like:

go run .
Thanks for using Pgxcli.
See you next time.
[error: failed to close the database connection]

We have to decide whether farewell message should be printed or not, if close error occurs. You can address this in another PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 584a3e9. Cleanup now attempts to close the application, database client, and logger even if an earlier close fails, then returns all close errors with errors.Join. For the farewell behavior, I chose to preserve #110’s intent: if any cleanup step fails, no farewell is printed. Added tests for cleanup order, error aggregation, and farewell behavior.

@dcavalcante dcavalcante changed the title fix(cli): print farewell after successful cleanup fix(cli): complete cleanup before farewell Aug 19, 2026
@dcavalcante
dcavalcante force-pushed the fix/cli-farewell-after-cleanup branch from 06a0082 to 01337d3 Compare August 19, 2026 20:35
@dcavalcante dcavalcante changed the title fix(cli): complete cleanup before farewell fix(cli): print farewell after successful cleanup Aug 19, 2026
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.

2 participants