-
Notifications
You must be signed in to change notification settings - Fork 183
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
Lua redis.log implementation #266
Conversation
* Added redis.log function support, wired to the standard logging lib
2 similar comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks like a solid start. I'd just like some extra handling of cases where the user uses it incorrectly.
'INFO:%s:verbose' % logger.name, | ||
'INFO:%s:notice' % logger.name, | ||
'WARNING:%s:warning' % logger.name]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another test (or several) which makes malformed calls e.g.
- Too many arguments
- Too few arguments (or no arguments)
- Message is not a string
- Level is not redis.LOG_*
Added tests and fixed multiarg calls and type conversion. Redis actually does some strange things with types. String, int and float are all converted to string, while boolean is just ignored. |
0: logging.DEBUG, | ||
1: logging.INFO, | ||
2: logging.INFO, | ||
3: logging.WARNING | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the double indirection because redis.LOG_DEBUG has the value 0 in real redis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and because user can actually use numbers instead of redis.LOG_*
variables, just like in logging
, but level values are different
fakeredis/_server.py
Outdated
msg = ' '.join([x.decode('utf-8') | ||
if isinstance(x, bytes) else str(x) | ||
for x in args if not isinstance(x, bool)]) | ||
LOGGER.log(REDIS_LOG_LEVELS_TO_LOGGING.get(lvl, logging.WARNING), msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use get
rather than just []
, given that you've already checked that lvl
is in REDIS_LOG_LEVELS.values()
?
test_fakeredis.py
Outdated
@@ -4178,6 +4178,45 @@ def test_lua_log(self): | |||
'INFO:%s:notice' % logger.name, | |||
'WARNING:%s:warning' % logger.name]) | |||
|
|||
def test_lua_log_no_message(self): | |||
if not isinstance(self.redis, (fakeredis.FakeRedis, fakeredis.FakeStrictRedis)): | |||
pytest.skip("Works only on fakeredis, as testing real redis will require access to its logs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this skip necessary? Won't real redis raise the same ResponseError? Same applies to the other tests that expect a ResponseError.
test_fakeredis.py
Outdated
|
||
def test_lua_log_defined_vars(self): | ||
if not isinstance(self.redis, (fakeredis.FakeRedis, fakeredis.FakeStrictRedis)): | ||
pytest.skip("Works only on fakeredis, as testing real redis will require access to its logs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code gets copy-pasted, it would be nice if it could be wrapped up into a decorator (so that it's stylistically similar to @redis3_only
for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now! I'll try to get around to cutting a new release in the next week or so.
Added a very simple
redis.log
Lua call support, wired to the standard pythonlogging
lib.Issue #265