Update front storage with back storage when cache only found in back storage #84
Conversation
|
@kirmanie, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vadymmarkov and @RamonGilabert to be potential reviewers. |
kirmanie
force-pushed
the
update-cache-logic
branch
from
9df0bfb
to
ef788c9
Compare
… time to cache object lookup
|
@kirmanie Great work 👍 Just a minor thing - could you please change to 2 space indentation, so it's aligned with the code style of the library? |
|
What do you guys think? @onmyway133 @zenangst |
|
|
||
| do { | ||
| let attributes = try fileManager.attributesOfItem(atPath: filePath(key)) | ||
| let fileModifiedDate = Expiry.date(attributes[FileAttributeKey.modificationDate] as! Date) |
There was a problem hiding this comment.
Can modificationDate be nil at this point?
There was a problem hiding this comment.
@zenangst I assume modificationDate can't be nil. But in the off chance that it is, if the file is deleted fileManager.attributesOfItem is called, I concluded that we shouldn't return a cache object.
There was a problem hiding this comment.
@kirmanie nice! was just curious :)
There was a problem hiding this comment.
@kirmanie I think I would still add guard or if to avoid force unwrapping.
|
Made one small comment. More of a question really, I like this, kudos @kirmanie |
|
Looks good to me |
|
@kirmanie mind fixing up the indentation so that we can merge this badboy? =) |
|
@zenangst I had one other suggested pattern for exposing the metadata. Please check out the latest commit. Let me know if it's too much against the overall vision and I can roll it back; the re-caching front storage was the feature I needed most. I'll fix the indentation right now. |
|
@zenangst @vadymmarkov @onmyway133 thanks for the feed back all. I'm really loving the library and how beautifully the code was written; really easy to read, really easy to contribute! |
|
First I'd just like to say; #themfeels
We really appreciate this kind of feedback ❤️ |
kirmanie
force-pushed
the
update-cache-logic
branch
from
072fd66
to
c8f0026
Compare
|
🚀 |
| - Parameter key: Unique key to identify the cache entry in the cache | ||
| - Parameter completion: Completion closure returns cache entry or nil | ||
| */ | ||
| public func cacheEntry(_ key: String, completion: @escaping (_ object: CacheEntry<T>?) -> Void) { |
There was a problem hiding this comment.
Does it need to be public?
There was a problem hiding this comment.
@vadymmarkov I made it public because I wanted a method that would return, not just the object, but object and metadata; I have code that needs to read the expiry time of a cached object (that hasn't expired) and make decisions about how it can be used based on how old it is. Does this sound like a use case you guys want to support?
I wasn't sure if we should add a new method to the Cache public interface or have an overload for the object method that takes a completion lambda with a CacheEntry parameter. The side effect of the later is we lose a bit of the swift inference when passing a lambda to the object method; users will have to be explicit with their parameter types which means a breaking change. Hence the new method.
There was a problem hiding this comment.
It makes sense @kirmanie, I just find cacheEntry vs object a bit confusing, but don't really have a better name 😄
There was a problem hiding this comment.
@vadymmarkov yeah agreed! I wrestled with the name a bit. First I thought objectEntry but that means the same as object. And objectMetadata seemed too specific; felt like leaving a little wiggle room since it was an addition to the public interface. Maybe a better name might come to one of us in a dream, lol. Thanks for getting those changes in though; I'll be sure to open PRs/issues for anything I find as I continue to use this great library.
|
@kirmanie amazing job here mate! |
|
@zenangst thanks for the feedback and thanks for merging in those changes! This library has been working nicely 😃 |
This PR is in response to issue 88. Just sharing an idea of how we can add support for the mentioned feature.