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

Support full Redis IANA URI scheme #1842

Merged
merged 2 commits into from Apr 2, 2018

Conversation

Projects
None yet
2 participants
@wilzbach
Contributor

wilzbach commented Jul 15, 2017

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 password part 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 again url is a very commonly used variable.

}
/// ditto
RedisDatabase connectRedisDB(string host_or_url)

This comment has been minimized.

@wilzbach

wilzbach Jul 15, 2017

Contributor

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

way to set configuration parameters.
*/
default:
cli.setConfig(param, val);

This comment has been minimized.

@wilzbach

wilzbach Jul 15, 2017

Contributor

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.

This comment has been minimized.

@s-ludwig

s-ludwig Jul 15, 2017

Member

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.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Jul 15, 2017

"https://google.com".url

I've got to say that I don't like these kinds of mini optimizations a lot. URL("https://google.com") is just a single character more to type and you can even do "https://google.com".URL if you want. Keeping the API slim and readable is IMO in most cases more important than saving a tiny bit of typing.

if(host_or_url.startsWith("redis://")){
return connectRedisDB(URL(host_or_url));
} else {
return connectRedis(host_or_url).getDatabase(0);

This comment has been minimized.

@s-ludwig

s-ludwig Jul 15, 2017

Member

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.

This comment has been minimized.

@wilzbach

wilzbach Jul 18, 2017

Contributor

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.

way to set configuration parameters.
*/
default:
cli.setConfig(param, val);

This comment has been minimized.

@s-ludwig

s-ludwig Jul 15, 2017

Member

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.

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Jul 18, 2017

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.

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Jul 18, 2017

Okay I moved the controversial convenience overloads to a new PR (#1852), s.t. nothing should be blocking this PR.

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Nov 30, 2017

@s-ludwig this still has the "Needs input" label, but AFAICT this PR has all comments addressed. Anything I am missing?

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Nov 30, 2017

Note that this would fix #1896

@wilzbach wilzbach removed the needs input label Apr 2, 2018

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Apr 2, 2018

Ping @s-ludwig I believe this got lost in the queue and nothing is blocking it.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Apr 2, 2018

True! Merging.

@s-ludwig s-ludwig merged commit d4df514 into vibe-d:master Apr 2, 2018

3 of 4 checks passed

codecov/patch 0% of diff hit (target 60.209%)
Details
codecov/project 60.631% (+0.421%) compared to e424042
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment