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

Document unix:// paths and add test for these #21

Closed
wants to merge 2 commits into from

Conversation

plaes
Copy link
Contributor

@plaes plaes commented May 5, 2015

Add note about unix:/// configuration support to README.

PS. rediss:// is also parsed properly but I currently don't have setup for that.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling d23c958 on plaes:master into 8ced34c on underyx:master.

@plaes
Copy link
Contributor Author

plaes commented May 5, 2015

Is the test server running custom redis-py which disallows non-'redis:.//' urls?

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling d23c958 on plaes:master into 8ced34c on underyx:master.

underyx pushed a commit that referenced this pull request Jul 18, 2016
@underyx
Copy link
Owner

underyx commented Jul 18, 2016

Actually I think the issue here was that unix:// URL support was only added in redis-py 2.7.x, and the CI suite checks 2.6.x as well. The other builds failed because of a PEP8 violation. I'm now committing this change manually to https://github.com/underyx/flask-redis/tree/release/0.3.0 and it should make it into master real soon. Sorry about the long, long wait! Two things to note:

  • The test seems largely unnecessary here; I think as it's just testing whether redis-py's URL parsing works correctly it does not need to be included with this project. The test result is completely independent of Flask-Redis. Therefore, I'm not including that in the commit.
  • Your second commit (d23c958) does not seem to be fixing an issue to me. The original form should be correct (and perhaps a tiny bit nicer) as well.

Since I basically just copied your change, I'm setting you as the author of the commit. You can see it attributed to you here: d27af44

Thanks a lot for the contribution! 👍

@underyx underyx closed this in be10bab Jul 18, 2016
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.

3 participants