Fix RedisCacheStore#write_multi with :expires_in
#49974
Conversation
|
Wdyt about enabling a relevant rubocop's cop for this in a follow up PR - https://docs.rubocop.org/rubocop/1.57/cops_lint.html#lintunusedmethodargument ? With it, this bug would be detected earlier. |
There was a problem hiding this comment.
Looks like this was accidentally broken in f9fce85#diff-8fbbb6f4480a782f49dbae5d364199ea4242fff5f6bd36c3bb01e36bed060b09L411.
| key = SecureRandom.uuid | ||
| @cache.write_multi({ "#{key}" => 1 }, expires_in: 10) | ||
|
|
||
| Time.stub(:now, time + 11) do |
There was a problem hiding this comment.
Is there a reason to not use travel here?
There was a problem hiding this comment.
Yeah. Copy paste from the above test.
Just noticed another problem - "#{key}" , interpolation is not needed. A work for another rubocop cop Style/RedundantInterpolation. I noticed a few offenses of it in rails. Do you think its worth enabling?
There was a problem hiding this comment.
Personally, no. Although I can't remember specific examples, I know there have been times when I preferred to write "#{foo}" instead of foo.to_s for stylistic reasons.
Also, given Ruby's lack of static typing, it seems like such a cop might not be very reliable.
There was a problem hiding this comment.
Yeah, no problems. This cop just detects patterns like "#{foo}" and suggests to replace them with foo.to_s (as a safe option), but many times this can be just written as foo.
I think it would depend on what the end result looks like for the entire Rails code base. Personally, I am not a fan of prefixing arguments with |
fatkodima
force-pushed
the
fix-redis-write_multi-with-expires_in
branch
from
ad0c89b
to
096201f
Compare
|
Thank you, @fatkodima! ✍️ Backported to |
…xpires_in Fix `RedisCacheStore#write_multi` with `:expires_in` (cherry picked from commit c36f877)
Fixes #49973.
This options were extracted as keyword arguments and not used then.