tpope / vim-dadbod

dadbod.vim: Modern database interface for Vim
https://www.vim.org/scripts/script.php?script_id=5665
3.75k stars 132 forks source link

URL param support for redis adapter #131

Open guruor opened 1 year ago

guruor commented 1 year ago

This should be better alternative to #129

Test case:

:echo db#adapter#dispatch("redis://localhost:6482/0?c&no-auth-warning", "interactive")

Result:

['redis-cli', '-c', '--no-auth-warning', '-h', 'localhost', '-p', '6482', '-n', '0']
tpope commented 1 year ago

This is indeed a better approach. But we should limit it to connection parameters; half or more of these (e.g., eval) have no business being in a connection URI. And we don't need single character aliases.

guruor commented 1 year ago

@tpope I just went through the redis-cli --help to get the non-boolean flags and added here. You are right we can limit to only connection parameters. I think few flags like cert, key, cacert, capath and tls-ciphers should be sufficient. I might be missing some crucial flags here but we can add those later when there is a need of it.

I added single character flag support because few of the flag are single character but not alias like u,r,i,d,D and c.

guruor commented 1 year ago

@tpope Have added the suggested changes, please review once again and let me know if any other change is required.

tpope commented 1 year ago

Didn't notice those single character options weren't aliases. But they don't really seem necessary to me, with the possible exception of -c. I would drop all of them; if someone wants -c we can add it back.

guruor commented 1 year ago

@tpope Removed the other single char flags (u, r, i, d and D) and weren't explicitly handling the -c anyways. Now we are good to go.

hiberabyss commented 1 year ago

Options like no-auth-warning should be set globally. Set it for every connection is tiresome.

tpope commented 1 year ago
  --no-auth-warning  Don't show warning message when using password on command
                     line interface.

Reading this description, yes, the fix for this option isn't a configuration option or a URL parameter, we should just pass it by default (or better, fix it to pass the password as an environment variable).

hiberabyss commented 1 year ago

Pass password by environment is a good idea. The mysql warning could also be suppressed via environment password like MYSQL_PWD=pass mysql -u user -h host.

guruor commented 1 year ago

@tpope Had pushed the updated changes, I think it is ready to merge now. let me know if you still have any suggestion or concerns.

guruor commented 1 year ago

@tpope Were you able to check the new changes?

guruor commented 1 year ago

@tpope bumping it once again, apologies for spamming. Just wanted to make sure if it being checked.