URL param support for redis adapter #131
Conversation
|
This is indeed a better approach. But we should limit it to connection parameters; half or more of these (e.g., |
|
@tpope I just went through the I added single character flag support because few of the flag are single character but not alias like |
|
@tpope Have added the suggested changes, please review once again and let me know if any other change is required. |
|
Didn't notice those single character options weren't aliases. But they don't really seem necessary to me, with the possible exception of |
- These are not really required - We mainly needed to handle -c for cluster mode, which is already being handled by the else case
|
@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. |
autoload/db/adapter/redis.vim
Outdated
| " Specifying only connection releated flag here, missing flags can be added later | ||
| if k =~# '^\%(cert\|key\|cacert\|capath\|tls-ciphers\|tls-ciphersuites\)$' && v isnot# 1 | ||
| call add(cmd, '--' . k . '=' . v) | ||
| elseif v =~# '^[1Tt]$' |
There was a problem hiding this comment.
This would make sense to have a k check too, no? That is k ==# 'tls' && v =~# '^[1Tt]', or something similar.
There was a problem hiding this comment.
Idea was to check the non-boolean flags explicitly and else case would always handle boolean flags.
There was a problem hiding this comment.
Is there an upside to doing it that way? The downside would be it could potentially pass a non-Boolean option as Boolean, resulting in a potentially confusing error message.
There was a problem hiding this comment.
No upside, just some laziness in handling it 😉 , will add the parameters mentioned in help doc explicitly.
|
Options like |
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). |
|
Pass password by environment is a good idea. The mysql warning could also be suppressed via environment password like |
- Added appropriate error message with instruction to check the `dadbod-redis` help
|
@tpope Had pushed the updated changes, I think it is ready to merge now. |
|
@tpope Were you able to check the new changes? |
|
@tpope bumping it once again, apologies for spamming. Just wanted to make sure if it being checked. |
doc/dadbod.txt
Outdated
| @@ -169,6 +169,9 @@ Redis ~ | |||
| Redis doesn't have a username, so use a dummy one in the URL if a password is | |||
| required. | |||
|
|
|||
| Query parameters such as `c`, `cert`, `key`, `cacert`, `capath`, `tls-ciphers` and | |||
There was a problem hiding this comment.
This line is too long. Use textwidth=78, as seen in the modeline.
| elseif k ==# 'c' && v =~# '^[1Tt]$' | ||
| " Some non-alias single char flags like `-c` needs to be passed | ||
| " with single hyphen `-` char | ||
| if len(k) == 1 |
There was a problem hiding this comment.
This check is redundant, as k ==# 'c' implies len(k) == 1.
| " Specifying only connection releated flag here, missing flags can be added later | ||
| if k =~# '^\%(cert\|key\|cacert\|capath\|tls-ciphers\|tls-ciphersuites\)$' && v isnot# 1 | ||
| call add(cmd, '--' . k . '=' . v) | ||
| elseif k ==# 'c' && v =~# '^[1Tt]$' |
There was a problem hiding this comment.
Drop the $ so that c=true can be used.
| else | ||
| throw 'DB: unsupport URL param `' . k . '` in URL ' . a:url . ', Check `:help dadbod-redis`' |
This should be better alternative to #129
Test case:
Result: