search

jazzband / dj-database-url

Use Database URLs in your Django Application.
https://pypi.org/project/dj-database-url/
BSD 3-Clause "New" or "Revised" License
1.49k stars 205 forks source link

Issue 68 redis support #69

Closed johntellsall closed 8 years ago

johntellsall commented 8 years ago

https://github.com/kennethreitz/dj-database-url/issues/68

includes tests

jdp commented 8 years ago

django-cache-url is probably a more appropriate place for this functionality, and Redis support is already there.

f0r4y312 commented 8 years ago

@jdp django-cache-url looks great; should have checked that before rolling my own

johntellsall commented 8 years ago

@jdp thanks for the django-cache-url ref!

In general I'd like my projects to have fewer, well-supported libraries. I'm hoping that this patch is accepted so I can just use dj-database-url for both. The other has had two patches in the last year.

I'll fix the issues that the CI found.

kennethreitz commented 8 years ago

Ugh.... this is out of scope. But, seemingly harmless. I'll take the risk. :)

kennethreitz commented 8 years ago

@johntellsall can you update the readme too?

kennethreitz commented 8 years ago

Once done, I'll push a release to PyPi and cut a release.

jdp commented 8 years ago

Does the returned dictionary actually work inside of a DATABASES config clause? It wouldn't work in a CACHES clause because the keys are different ('ENGINE' vs 'BACKEND', 'HOST' vs 'LOCATION', etc.)

benwilber commented 8 years ago

I'm not sure what this redis support is actually intended to do? It won't work as a DATABASES setting and it won't work as a CACHES setting (as @jdp pointed out).

f0r4y312 commented 8 years ago

@benwilber About a year ago, heroku made a redis add-on available and they use REDIS_URL env variable to store the cache config similar to the DATABASE_URL, so for someone using dj-database-url becuase they are on heroku, I guess it would make sense to have redis support as well. But I do agree the cache config dict is different from the database config dict.

kennethreitz commented 8 years ago

Looks like this may need to be updated w/ BACKEND, LOCATION, etc for CACHES use (which is what it is intended for).

johntellsall commented 8 years ago

@kennethreitz , based on the (quite correct) criticism from @jdp and others I suggest rolling back this patch.

Here's the original use case: in our Django project we use Redis for several things, each one in a different "database index." For example redis://host/0 for Django cache, redis://host/1 for internal synchronization, redis://host/2 for Celery, etc. As a dev I want to have a single URL passed in from outside, and the settings file would override the index for each of the internal users of Redis. Since dj-database-url parses DSN URLs, I used it.

As it turns out, I was wrong. Since neither this library nor django_cache_url has a format() command, it's awkward to take the main Redis URL and replace just the database index. I could update this patch with BACKEND etc, but that would directly overlap with django_cache_url.

I suggest zapping this patch. Sorry for the confusion.

kennethreitz commented 8 years ago

I think there's no harm in duplicating, partially, the behavior of django_cache_url at all. However, there has been almost no demand for that, so I'm happy to say there is no need.

kennethreitz commented 8 years ago

https://github.com/kennethreitz/dj-database-url/commit/afc4d3946c041ad0f5c70609a6d5e4e39ff54919