Add method Cell#cached? #478
Conversation
PikachuEXE
force-pushed
the
add-method-cached-to-cell
branch
from
68bb0ad
to
11389ca
Compare
|
Spec fixed |
|
Hello any more change required? |
|
LOL almost forgot about this PR |
|
Yo? |
|
Hello? |
|
@seuros :) ? |
There was a problem hiding this comment.
Sorry, this can't be approved until we remove the redundancy from Caching#render_state, there are two really complex identical lines. Also, I'm a bit nervous about cache_store.exist?(key, options) since some people might use e.g. a Hash cache store? Why don't you just fetch the content the way it's done in render_state to see if it's cached?
lib/cell/caching.rb
Outdated
| def cached?(state=:show, *args) | ||
| return false unless cache?(state, *args) | ||
|
|
||
| key = self.class.state_cache_key(state, self.class.version_procs[state].(self, *args)) |
There was a problem hiding this comment.
I feel like this is repeating code from this file?
There was a problem hiding this comment.
Line 70-71 are repeating already existing code. This is not a good idea, we should extract it to one method if this gets approved.
There was a problem hiding this comment.
Method extracted
| end.new | ||
|
|
||
| def with_cache_cleanup |
There was a problem hiding this comment.
What is the purpose of this method, I wonder? 🤔
There was a problem hiding this comment.
There is no after(:each) like rspec
So add this for cleanup after each test
lib/cell/caching.rb
Outdated
| key = self.class.state_cache_key(state, self.class.version_procs[state].(self, *args)) | ||
| options = self.class.cache_options[state].(self, *args) | ||
|
|
||
| cache_store.exist?(key, options) |
There was a problem hiding this comment.
Where does #exist? come from? Is that the official name in cache stores, or should there rather be key? for Hash-compat?
There was a problem hiding this comment.
|
Do you have any example on where this method is used in your apps? 🍻 |
PikachuEXE
force-pushed
the
add-method-cached-to-cell
branch
from
11389ca
to
fd64505
Compare
|
I mainly use this method for
|
|
Build failed in 2.2 due to No test case is failing |
PikachuEXE
force-pushed
the
add-method-cached-to-cell
branch
from
d5471fc
to
5e309c3
Compare
|
Build for ruby 2.2 fixed |
|
Anything else I should respond to or update? |
|
Hello again :3 ? |
|
Thanks @PikachuEXE for your contributions! ❤️ We are still mulling over the decision whether or not this addition is really needed in the core code. It's a helpful method, but every method added is a method you need to maintain, test, and deprecate at some point. The more public API an object has, the worse it gets (see ActiveRecord for reference). One idea I have is we could have a cells-cached gem and still merge your internal refactorings in cells core. With the download statistics we can then analyze how much this addition is being used - I am guessing, it won't be used a lot (#nojudging ;). I apologize if that feels "unfair" but I'm planning a very slim Cells 5 and the more we add now, the more we have to deprecate, and in all fairness, there are more pressing issues than a caching tester, so please bear with me and think about my suggestions again. |
|
I am totally fine with having an extension gem instead of merging it into core. |
|
Also to let people know about your plan of releasing slim gem(s) |
This is for logging cache hit rate for specific cells
Looking at caching server hit rate is useless and also inaccurate since rails 5.2's cache versioning