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

Refuse to quit cli if a transaction is ongoing. #1400

Merged

Conversation

dbaty
Copy link
Member

@dbaty dbaty commented Mar 29, 2023

Description

If a transaction is ongoing, quitting the cli (with "ctrl-d" or "quit") is not possible anymore. An error message is displayed. The user is forced to commit or rollback the transaction before quitting.

Fixes #1071.

Notes

  1. I did not add a new configuration option. This commit thus changes the current behaviour of pgcli (for good, I believe). This is debatable, I could add a configuration option (off by default) if preferable.
  2. I added a "behave" test. This is my first one, feel free to comment if it's not the right way to do it.
  3. Demo: asciicast

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 am already in this file.)
  • 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)

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Patch coverage: 52.62% and project coverage change: -4.74% ⚠️

Comparison is base (6884c29) 84.15% compared to head (5f38269) 79.42%.
Report is 62 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1400      +/-   ##
==========================================
- Coverage   84.15%   79.42%   -4.74%     
==========================================
  Files          21       25       +4     
  Lines        2720     3008     +288     
==========================================
+ Hits         2289     2389     +100     
- Misses        431      619     +188     
Files Changed Coverage Δ
pgcli/completion_refresher.py 91.76% <ø> (+1.17%) ⬆️
pgcli/magic.py 0.00% <0.00%> (ø)
pgcli/packages/parseutils/tables.py 97.67% <ø> (ø)
pgcli/packages/sqlcompletion.py 97.67% <ø> (ø)
pgcli/pgcompleter.py 96.95% <ø> (ø)
pgcli/pgstyle.py 64.00% <ø> (ø)
pgcli/pyev.py 15.38% <15.38%> (ø)
pgcli/pgtoolbar.py 25.71% <20.00%> (-5.72%) ⬇️
pgcli/explain_output_formatter.py 46.15% <46.15%> (ø)
pgcli/key_bindings.py 52.94% <66.66%> (-0.19%) ⬇️
... and 8 more

... and 1 file with indirect coverage changes

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

@j-bennet
Copy link
Contributor

j-bennet commented Mar 31, 2023

@dbaty Thank you for the PR! Very nice work, you went above and beyond, what with the behave test, and the demo.

I think I would do things a little differently. If the user tries to quit and a transaction is in progress, I would output a message as follows:

A transaction is ongoing. Choose c to COMMIT or r to ROLLBACK Default: [r] >

If the user presses ENTER, transaction is rolled back, and pgcli exits.
If the user types r and presses ENTER, same.
If the user types c and presses ENTER, transaction is rolled back, and pgcli exits.
If the user types anything else besides r or c, same message is repeated.

It's slightly more complicated to implement, but nicer user experience, because if the user is ok with losing their changes, they can just press ENTER once more to quit, without typing anything.

What do you think?

@dbaty
Copy link
Member Author

dbaty commented Mar 31, 2023

That makes sense. I'll try to implement that next week.

If user tries to quit the cli while a transaction is ongoing (i.e.
begun, but not committed or rolled back yet), pgcli now asks for a
confirmation. The user can choose to commit, rollback or cancel the
exit. If the user chooses to commit or rollback, we exit only if the
commit/rollback succeeds.

Fixes dbcli#1071.
@dbaty dbaty force-pushed the dbaty/warn_upon_exit_if_non_committed_txn branch from fedd7b7 to 5f38269 Compare September 13, 2023 06:43
@dbaty
Copy link
Member Author

dbaty commented Sep 13, 2023

I'll try to implement that next week.

Yeah, right. By "next week", I actually meant "within 6 months"... Anyway, it's done.

Notes:

  • I struggled a bit to add Behave tests but they pass. Suggestions are welcome.
  • @j-bennet : I slightly adapted your proposal to add a way to cancel the exit. Use case: I have an ongoing transaction, I type "Ctrl-D" because I forgot what I was doing (or because I have very clumsy fingers), but I don't want to commit nor rollback yet. I can "cancel" the exit and do more stuff before a commit or rollback.

I recorded a new demo:

asciicast

@j-bennet
Copy link
Contributor

Hi @dbaty, this looks great, and works as expected. Thanks a lot, great feature! Merging. 🍫

@j-bennet j-bennet merged commit cdfa358 into dbcli:main Sep 27, 2023
7 checks passed
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.

Warning of existing transaction when quitting pgcli (asking for COMMIT or ROLLBACK)
3 participants