-
Notifications
You must be signed in to change notification settings - Fork 334
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
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can modificationDate
be nil at this point?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirmanie nice! was just curious :)
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.
@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! |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be public
?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.