Skip to content
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

Add readBlock to objectForKey & writeBlock to setObject:forKey: #6

Closed
wants to merge 15 commits into from

Conversation

dimazen
Copy link

@dimazen dimazen commented Jun 8, 2015

Generally this is a merge of https://github.com/tumblr/TMCache/pull/30/commits to PINCache.
Read & Write blocks are quite useful when object isn't compatible with NSCoding or manual action needs to be performed

@jparise
Copy link
Collaborator

jparise commented Jun 8, 2015

Thanks for the contribution! Would you be willing to split this up into multiple pull requests (e.g. one that adds nonnull, etc. attributes to the current code, one that adds the new read/write blocks, etc.)? That would make it easier to review the functional changes on their own.

@dimazen
Copy link
Author

dimazen commented Jun 8, 2015

@jparise should I remove nullability attributes and then make a diff branch with them?
What is the best way to do it?

@jparise
Copy link
Collaborator

jparise commented Jun 8, 2015

Yes, that would be my preference. @garrettmoon is the primary maintainer, though, so let's see what he says.

@garrettmoon
Copy link
Collaborator

Hi Dima,

Thanks so much for the contribution!

I agree with Jon that splitting this up into multiple pull requests will make it easier to review and merge in. If you're willing to do that, it would be greatly appreciated!

On Mon, Jun 8, 2015 at 9:55 AM, Jon Parise notifications@github.com
wrote:

Yes, that would be my preference. @garrettmoon is the primary maintainer, though, so let's see what he says.

Reply to this email directly or view it on GitHub:
#6 (comment)

@dimazen
Copy link
Author

dimazen commented Jun 8, 2015

I'll do my best tomorrow's morning ;)

@dimazen
Copy link
Author

dimazen commented Jun 10, 2015

Sorry, @garrettmoon, I'm a bit busy atm, so waiting for the weekends

@garrettmoon
Copy link
Collaborator

Hey, no worries!

@dimazen
Copy link
Author

dimazen commented Jun 21, 2015

Guys, having a really crazy pre-release weeks, sorry one more time

@dimazen
Copy link
Author

dimazen commented Jun 22, 2015

@garrettmoon I reverted nullability attributes, check it!

@garrettmoon
Copy link
Collaborator

Thanks! I'll give this another look by EOD Friday.

@@ -124,6 +140,18 @@ typedef void (^PINCacheObjectBlock)(PINCache *cache, NSString *key, id __nullabl
- (void)setObject:(id <NSCoding>)object forKey:(NSString *)key block:(nullable PINCacheObjectBlock)block;

/**
Stores an object in the cache for the specified key. This method returns immediately and executes the
passed block after the object has been stored, potentially in parallel with other blocks on the <queue>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be concurrentQueue

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste got me :)

@garrettmoon
Copy link
Collaborator

This looks good! Aside from the few nits above, there's one other thing I'd request: many of the changes are whitespace changes that seem unnecessary and conflict with the existing whitespace patterns. Do you mind cleaning this up?

@dimazen
Copy link
Author

dimazen commented Jul 6, 2015

@garrettmoon
Check out my fixes!

@dimazen
Copy link
Author

dimazen commented Jul 6, 2015

@garrettmoon
Aside this PR can you also check other branches, that I have, maybe some of them can be useful for master repo:
feature/extension - saves key extension to disk, when you're passing key like "key.png". This one is useful, when you're using QLPreviewController - it can't identify object content-type without extension.

feature/object_at_path - allows you to set object at path. This is useful, when object is relatively big, so instead of explicit read and write NSFileManager used to simply copy / move file.

feature/trim_by_keys - this one is quite specific. It allows you to trim a defined set of keys to some date (heavily used in my current project cleanup of unused key with specific criteria)

BlueprintName = "iOS"
ReferencedContainer = "container:PINCache.xcodeproj">
</BuildableReference>
</MacroExpansion>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? What does it do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange why it gets commited.
I was trying to run tests from iOS target by cmd+u, however this one should be reverted..

@garrettmoon
Copy link
Collaborator

@dimazen Nice! And if you feel that the features would be widely useful to others, feel free to submit pull requests. One more question and I think we're there!

@garrettmoon
Copy link
Collaborator

I think I dropped the ball on this. The last question was, can you reformat your patch so it doesn't include all the whitespace changes? I know this is a pain, but it will help keep our git history. Thank you in advance!

@dimazen
Copy link
Author

dimazen commented Jul 29, 2015

Argh, I've totally forgot about it, sure I will with no questions ;)

@garrettmoon
Copy link
Collaborator

Just to keep things clean, I'm going to close this. Feel free to reopen if you'd like to continue working on it.

@garrettmoon garrettmoon closed this Oct 2, 2015
@garrettmoon
Copy link
Collaborator

Or rather, let me know if you'd like to reopen :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants