fix disabling of distributed lock (#2)#109
fix disabling of distributed lock (#2)#109bertosantamaria wants to merge 1 commit intosibson:masterfrom
Conversation
|
Addresses #101 |
|
@sibson, please let me know if there is anything I can do/change here to help move this along. |
sibson
left a comment
sibson
left a comment
There was a problem hiding this comment.
I think this change is good but I'm struggling with understanding the semantics of has_value vs present. I understand one is converting None to default and the other preserves None but something in the naming isn't clicking in my head. Perhaps, adding a docstring to each to better describe their behaviour would help?
|
|
||
| def test_disable_lock_key_3(self): | ||
| self.app.conf.REDBEAT_LOCK_KEY = None | ||
| self.assertTrue("REDBEAT_LOCK_KEY" in self.app.conf.keys()) |
There was a problem hiding this comment.
Is this necessary? It seems to be testing the assignment we did in the previous line or am I missing some behind the scenes magic?
There was a problem hiding this comment.
Oh yes. I'll remove this.
| self.assertTrue("redbeat_lock_key" in self.app.conf.keys()) | ||
| self.conf = RedBeatConfig(self.app) | ||
| self.assertEqual(self.conf.lock_key, None) | ||
|
|
There was a problem hiding this comment.
Should there be a @pytest.mark.skipif(CELERY_4_OR_GREATER, reason="requires Celery < 4.x") here?
There was a problem hiding this comment.
Yup. Will add the skipif
There was a problem hiding this comment.
I've dropped celery 3 support so this maybe simpler to fix.
| def test_key_prefix_default(self): | ||
| self.assertEqual(self.conf.key_prefix, 'redbeat:') | ||
|
|
||
| def test_lock_key_default(self): |
There was a problem hiding this comment.
To check my understanding, is this the same test as in https://github.com/sibson/redbeat/pull/109/files#diff-423890b7e3c2aa4791f9e203b7f194cbR45? If so I don't think we need to remove it, being explicit here is great.
There was a problem hiding this comment.
Yes, this tests the same thing and was added to be explicit.
| self.assertTrue("REDBEAT_LOCK_KEY" in self.app.conf.keys()) | ||
| self.conf = RedBeatConfig(self.app) | ||
| self.assertEqual(self.conf.lock_key, None) | ||
|
|
There was a problem hiding this comment.
Should there be a test_custom_lock_key() or is that covered elsewhere somehow?
There was a problem hiding this comment.
Yup. I'll add a test for a custom lock key.
| self.warn_if_wrong_format(name) | ||
| return self.app.conf.first(name, name.upper()) or default | ||
|
|
||
| def key_present_or(self, name, default=None): |
There was a problem hiding this comment.
Should there be tests for this function?
There was a problem hiding this comment.
Definitely. I'll add one.
|
@sibson @bertosantamaria Any new on this? Can I help? This feature is really needed and currently not working. |
|
Looking at the comment history, this was close to being good to go just needed a few changes and now it has conflicts. If you'd like to start a new PR too address the feedback we could get it merged in favor of this. |
|
Will merge once conflicts are resolve.d |
3 participants
No description provided.