Support full Redis IANA URI scheme #1842
Conversation
redis/vibe/db/redis/redis.d
Outdated
| } | ||
|
|
||
| /// ditto | ||
| RedisDatabase connectRedisDB(string host_or_url) |
There was a problem hiding this comment.
This is inspired by the way mongo handles things, hence the weird underscore parameter name.
I would prefer sht. like hostOrURL though ;-)
redis/vibe/db/redis/redis.d
Outdated
| way to set configuration parameters. | ||
| */ | ||
| default: | ||
| cli.setConfig(param, val); |
There was a problem hiding this comment.
This might be controversial and I don't have a use case for it atm, I just thought it might be a nice feature, but I am happy to remove this bit.
There was a problem hiding this comment.
Is there precedence for other implementations for this? It does bring the risk of clashing with other official options besides "password" that might be added in the future.
I've got to say that I don't like these kinds of mini optimizations a lot. |
redis/vibe/db/redis/redis.d
Outdated
| if(host_or_url.startsWith("redis://")){ | ||
| return connectRedisDB(URL(host_or_url)); | ||
| } else { | ||
| return connectRedis(host_or_url).getDatabase(0); |
There was a problem hiding this comment.
Not sure if it's a good idea to imply database 0 here. I'd normally say that a choice like this should be explicit.
There was a problem hiding this comment.
As mentioned this is inspired by the way the Mongo adapter works and as connectRedisDB, returns a RedisDatabase, it has to be returned here as well.
redis/vibe/db/redis/redis.d
Outdated
| way to set configuration parameters. | ||
| */ | ||
| default: | ||
| cli.setConfig(param, val); |
There was a problem hiding this comment.
Is there precedence for other implementations for this? It does bring the risk of clashing with other official options besides "password" that might be added in the future.
No as mentioned it was just an idea -> replaced it with throwing an Exception about it being unsupported. |
|
Okay I moved the controversial convenience overloads to a new PR (#1852), s.t. nothing should be blocking this PR. |
|
@s-ludwig this still has the "Needs input" label, but AFAICT this PR has all comments addressed. Anything I am missing? |
|
Note that this would fix #1896 |
|
Ping @s-ludwig I believe this got lost in the queue and nothing is blocking it. |
|
True! Merging. |
For reference: many PaaS providers supply the Redis password directly in the URL and it took me quite a bit to figure out that the Redis implementation simply doesn't look at the
passwordpart of the Redis URI.Regarding this PR, I am not sure how add test for this that work on all machines. Happy about ideas.
I copied the interesting bits from the spec for future readers.
BTW what do you think about adding a convenient UFCS-friendly way to build URLs?
I would love to write:
"https://google.com".url, but then againurlis a very commonly used variable.