search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.26k stars 217 forks source link

Add feature to disable cooldown checking #223

Closed mycarrysun closed 5 years ago

mycarrysun commented 5 years ago

This PR fixes #209 by adding an option to completely disable all cooldown checking, or by individual classes

mikebronner commented 5 years ago

Thanks for working on this! I will review and try to get this (or whatever solution) integrated next week! I appreciate the help :)

mikebronner commented 5 years ago

Hi @mycarrysun, I went for a slightly simpler (or at least I believe it is) solution, and because of that didn't integrate your PR. However, please don't let that be a sign that I don't appreciate your work. Thanks for submitting this, it helped me in reviewing the functionality and coming up with a solution I felt good with. Thanks! :)

mycarrysun commented 5 years ago

Where are the changes that you implemented?

mikebronner commented 5 years ago

See my comment on issue #209 ... check latest release:

https://github.com/GeneaLabs/laravel-model-caching/commit/f8aafe23a5a8b815b2fddac0a9f9595bb56d6136

mycarrysun commented 5 years ago

Looks good to me - any reason you decided to go with a prop instead of a trait or in the config? Thinking people will be changing this at runtime?

mycarrysun commented 5 years ago

Also good work on this library!!! I can't believe how much it improved the load time on our app....I knew it would be an improvement but didn't expect that much!!

Was thinking another feature that might be handy for folks using the cooldown, set a default cooldown time on the Model. Then you don't have to call it on every query.

mikebronner commented 5 years ago

No, I wanted the setup to be declarative at the model level, since it's model-specific. I also did not want to introduce back-ward-incompatibility, I felt the config setting was an extra unnecessary step. Also, having empty traits on models to act as a flag seems odd to me, I would rather have a property be the flag, it is self-describing, you don't wonder what it does without having to go to a different file to look at it.

Using a default property will make it easy to implement for the few people that actually want to use it, and keep it completely out of the way of those that don't. :)

mikebronner commented 5 years ago

Good call with the default cool down. In fact, I can combine that with the feature flag. If cool down is 0, null, or false, then skip cooldown-checks, and if greater than 0, then run them. I like it!