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

Provide a convenience string overload for connectRedisDB #1852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Jul 18, 2017

Split-off from #1842

This is inspired by the way mongo handles things, hence the weird underscore parameter name.
I would prefer sth. like hostOrURL though ;-)

@@ -33,6 +33,7 @@ import std.utf;
*/
RedisClient connectRedis(string host, ushort port = RedisClient.defaultPort)
{
assert(!host.startsWith("redis://"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should leave a message what's wrong and why it's wrong for the user because this is a public function.

Something like host must be a valid IP or domain, but not an URI

@@ -33,6 +33,7 @@ import std.utf;
*/
RedisClient connectRedis(string host, ushort port = RedisClient.defaultPort)
{
assert(!host.startsWith("redis://", "Host must be a valid IP or domain, but not an URI."));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also think about parsing the host URL if the user accidentally called the wrong function, but imho it's better to use an error message here, because if the user has a full redis URI, he should have called connectRedisDB which returns a RedisDatabase and not only a RedisClient

@wilzbach
Copy link
Member Author

Added the error message to the assert. Anything else blocking this?

Copy link
Contributor

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never use redis (always mongodb) so I'm not sure about how consistent the getDatabase(0) part is with other implementations/apis but otherwise this LGTM as it is a pretty simple change

@wilzbach
Copy link
Member Author

wilzbach commented Apr 2, 2018

I never use redis (always mongodb) so I'm not sure about how consistent the getDatabase(0)

It's the default database like Mongo's default default.

Anyhow, in case it wasn't clear this just unifies the API with how the analog connectMongoDB works:

MongoClient connectMongoDB(string host, ushort port)
{
assert(!host.startsWith("mongodb://"));
return new MongoClient(host, port);
}
/// ditto
MongoClient connectMongoDB(string host_or_url)
{
/* If this looks like a URL try to parse it that way. */
if(host_or_url.startsWith("mongodb://")){
return new MongoClient(host_or_url);
} else {
return new MongoClient(host_or_url, MongoClientSettings.defaultPort);
}
}

Ping @s-ludwig

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.

2 participants