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

Bump target to iOS7, introduce nullability, always async, and separate storage from queue #22

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

gelosi
Copy link

@gelosi gelosi commented Feb 19, 2016

Hi! Have a look please

I've updated README accordingly. Idea is:

  • introduce QueueJob to avoid direct using NSDictinary for creating jobs (plan to add to Job max retry times, expiration date, fire date, etc)
  • introduce nullability to prepare for Swift migration
  • decoupling storage from queue using EDQueuePersistentStorage brotocoli :)
  • adding some basic tests on top of that (to be continued)

added nullability

added EDQueueJob instead of dictionary + keys (for user data it still provides userInfo dictionary, nullable)
Updated Code & Example

Added basic Queue tests

README updated accordingly
@thisandagain
Copy link
Owner

Very cool. Taking a look now.

s.authors = {'Andrew Sliwinski' => '[email protected]', 'Francois Lambert' => '[email protected]'}
s.source = { :git => 'https://github.com/thisandagain/queue.git', :tag => 'v0.7.1' }
s.platform = :ios, '5.0'
s.homepage = 'https://github.com/gelosi/queue'
Copy link
Owner

Choose a reason for hiding this comment

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

Why would we move the location of the homepage and repo?

Copy link
Author

Choose a reason for hiding this comment

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

I've set it to mine now to use it already, but if you will accept the changes in the end (I'm still on it) I'd be happy to keep original link to repo.

@thisandagain
Copy link
Owner

Overall this is great. Thanks for pulling this together @gelosi . I left you a few comments and notes for discussion.

@gelosi
Copy link
Author

gelosi commented Feb 19, 2016

Hi Andrew

Cool! Thank you for a quick feedback. This is a very start. I will try to offer some idea on 'built-in' persistent storage. As I suppose apps which using  already some sqlite wrapper different from FMDB might stay aside. That is my approach. Ideally - having ready-to go fmdb submodule (as default dependency). And reduce persistent storage interface requirements to CRUD :)


RO

On Fri, Feb 19, 2016 at 7:04 PM, Andrew Sliwinski
[email protected] wrote:

Overall this is great. Thanks for pulling this together @gelosi . I left you a few comments and notes for discussion.

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

@gelosi
Copy link
Author

gelosi commented Feb 24, 2016

@thisandagain so, shaping better approach. Have a look, please.

…ueue lost retryCount respectively; More Tests!
…etchNextJobTimeInterval`). Also, fixed critical bug - a race condition in queue. So, job might be executed two times :/ Hola TDD!
@end


NS_ASSUME_NONNULL_END

Choose a reason for hiding this comment

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

New line missing

@property(nonatomic, readonly) EDQueueJob *job;

@property(nonatomic, readonly, nullable) NSNumber *jobID;
@property(nonatomic, readonly, nullable) NSNumber *attempts;

Choose a reason for hiding this comment

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

Let's just use @0 as a default value and avoid nil.

Copy link
Author

Choose a reason for hiding this comment

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

👍

…EDQueueStorageItem protocol (and EDQueueStorageEngineJob respectively), blocked +new method from EDQueueJob ^^,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants