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

Add verbose_errors config and special command #1455

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Add verbose_errors config and special command #1455

merged 5 commits into from
Mar 26, 2024

Conversation

amacfie
Copy link
Contributor

@amacfie amacfie commented Mar 19, 2024

Description

To address #1407, this adds a boolean parameter to the config file called verbose_errors. If set, the values of any postgres error fields present (see https://www.postgresql.org/docs/current/protocol-error-fields.html) are appended to the error message.

(The code that extracts the error fields should be good but I'm not sure if a config parameter is the best place to control this, or whether this PR does everything necessary to introduce a new config parameter.) ETA: I've also added the \v special command which toggles verbose errors.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

@j-bennet
Copy link
Contributor

j-bennet commented Mar 20, 2024

@amacfie This looks good. As far as a new config setting implementation, you did it right. As a pgcli user, what would be your pattern of using this feature?

Would you always keep it on? In that case, the config setting make sense.

Would you enable it on a per session basis? A new cli option might be the solution then, similar to this:

pgcli/pgcli/main.py

Lines 1372 to 1376 in 9f114c4

@click.option(
"--warn",
default=None,
help="Warn before running a destructive query.",
)

Would you only want to enable it for a certain query? In that case, you may think about implementing a special command, see for example \\T - changing the table format:

pgcli/pgcli/main.py

Lines 362 to 367 in 9f114c4

self.pgspecial.register(
self.change_table_format,
"\\T",
"\\T [format]",
"Change the table format used to output results",
)

Another pattern for switching quickly between on and off is adding a toolbar item:

pgcli/pgcli/pgtoolbar.py

Lines 31 to 34 in 9f114c4

if pgcli.multi_line:
result.append(("class:bottom-toolbar.on", "[F3] Multiline: ON "))
else:
result.append(("class:bottom-toolbar.off", "[F3] Multiline: OFF "))

In fact, for multiline mode, we do both: you can set it in the config file permanently, and still quickly switch modes in the toolbar. Based on our own experience as pgcli users, we appreciate the flexibility.

So you have many options. Think about what would work great for you.

@amacfie amacfie changed the title Add verbose_errors config Add verbose_errors config and special command Mar 21, 2024
@amacfie
Copy link
Contributor Author

amacfie commented Mar 21, 2024

Ok I've added a special command which should provide enough flexibility.

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 80.03%. Comparing base (a7a70fd) to head (3cc5f3f).

Files Patch % Lines
pgcli/main.py 82.14% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1455      +/-   ##
==========================================
- Coverage   80.07%   80.03%   -0.04%     
==========================================
  Files          25       25              
  Lines        3072     3126      +54     
==========================================
+ Hits         2460     2502      +42     
- Misses        612      624      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@j-bennet
Copy link
Contributor

@amacfie You have a conflict to resolve after your previous PR merge, and in CI, black is not happy.

@amacfie
Copy link
Contributor Author

amacfie commented Mar 26, 2024

@amacfie You have a conflict to resolve after your previous PR merge, and in CI, black is not happy.

Ok those are addressed. What's going on with the Python 3.12 integration tests?

@j-bennet
Copy link
Contributor

What's going on with the Python 3.12 integration tests?

Looks intermittent; this happens sometimes. pexpect weirdness.

@j-bennet j-bennet merged commit 8cc22b9 into dbcli:main Mar 26, 2024
7 checks passed
@j-bennet
Copy link
Contributor

@amacfie Merged; thank you!

@amacfie
Copy link
Contributor Author

amacfie commented Mar 27, 2024

Thanks for the quick reviews!

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.

None yet

3 participants