-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Issue #598 extend support for the hashmap functions #746
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #746 +/- ##
========================================
+ Coverage 63.3% 64.3% +1.1%
========================================
Files 43 43
Lines 3215 3389 +174
Branches 244 287 +43
========================================
+ Hits 2034 2179 +145
- Misses 1164 1192 +28
- Partials 17 18 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
looks great, but please add versioning in another PR so that it can be also tested separately
def test_hsetnx(self, cache: RedisCache): | ||
result_foo1 = cache.hsetnx("foo_hash1", "foo1", "bar1") | ||
result_foo2 = cache.hsetnx("foo_hash1", "foo2", "bar2") | ||
result_foo2_1 = cache.hsetnx("foo_hash1", "foo2", "bar2") |
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.
could you change the value written?
result_foo2_1 = cache.hsetnx("foo_hash1", "foo2", "bar2") | |
result_foo2_1 = cache.hsetnx("foo_hash1", "foo2", "bar3") |
@rootart do you have time to finish this PR? otherwise I can take it... that said, thanks for the effort you've put in this project so far, it is highly appreciated 😄 |
No description provided.