dbcli / pgcli

Postgres CLI with autocompletion and syntax highlighting
http://pgcli.com
BSD 3-Clause "New" or "Revised" License
12.11k stars 557 forks source link

"You're about to run a destructive command" for command that isn't destructive. #1013

Closed fake-name closed 5 years ago

fake-name commented 5 years ago
webarchive> ALTER TABLE web_pages ADD CONSTRAINT test_unique_url UNIQUE (url);
You're about to run a destructive command.
Do you want to proceed? (y/n):

Adding a constraint isn't destructive. Why am I getting a warning? Is pgcli doing something else when I add a constraint?

Your environment

durr@postgres-server ~> pip freeze
asn1crypto==0.24.0
attrs==17.4.0
Automat==0.6.0
bottle==0.12.13
certifi==2018.1.18
chardet==3.0.4
cli-helpers==1.1.0
click==6.7
colorama==0.3.7
configobj==5.0.6
constantly==15.1.0
cryptography==2.1.4
cycler==0.10.0
docker==2.5.1
docker-pycreds==0.2.1
Glances==2.11.1
humanize==0.5.1
hyperlink==17.3.1
idna==2.6
incremental==16.10.1
influxdb==4.1.1
iotop==0.6
matplotlib==2.1.1
netifaces==0.10.4
numpy==1.13.3
olefile==0.45.1
PAM==0.4.2
pgcli==2.0.2
pgspecial==1.11.5
Pillow==5.1.0
ply==3.11
prompt-toolkit==2.0.9
psutil==5.4.2
psycopg2==2.7.7
pyasn1==0.4.2
pyasn1-modules==0.2.1
pycryptodomex==3.4.7
pycurl==7.43.0.1
Pygments==2.3.1
pygobject==3.26.1
pyOpenSSL==17.5.0
pyparsing==2.2.0
pyserial==3.4
pysmi==0.2.2
pysnmp==4.4.3
pystache==0.5.4
python-apt==1.6.3+ubuntu1
python-dateutil==2.6.1
pytz==2018.3
PyYAML==3.12
requests==2.18.4
service-identity==16.0.0
setproctitle==1.1.10
six==1.11.0
sqlparse==0.2.4
tabulate==0.8.3
terminaltables==3.1.0
Twisted==17.9.0
unattended-upgrades==0.1
urllib3==1.22
wcwidth==0.1.7
websocket-client==0.44.0
zope.interface==4.3.2

I assume this is some sort of "double check on ALTER commands" thing, but well, it's sufficently dumb about it that it only serves to be annoying. Why was this even implemented?

If you're going to add warning popups with wording that make it sound like you're going to delete stuff, make sure it only happens when you're actually going to delete stuff!

If you can't fully disambiguate if a command is actually destructive, don't call them destructive commands.

I still think this entire feature is dumb, but maybe say "You're about to run a ALTER/Whatever command. This may irrevocably delete data. Continue?", or some similar verbiage that at least is aware that not all ALTER commands delete stuff.

amjith commented 5 years ago

Hi. You're right it is implemented naively. We simply check if it is an ALTER command display the warning.

I'm open to changing the verbiage, feel free to open a PR. If you want to turn off that warning you can also do that via your config file. Check for the words destructive_warning.