New task-scheduling infrastructure. #73
Conversation
This commit contains a major architectural revision: cleaning up our scheduling system. The original system scheduled tasks into the future; this system maintains the scheduling rules, but doesn't actually schedule anything until the user taps on it. We also: - centralized lots of code, making it easier to maintain and debug - made certain common features (schedule delays and task-expiration periods) data-driven instead of software-driven
|
Please remove OpenSSL. |
|
@p2 Could you please have a look at this from your point of view? |
|
Hmm, it doesn't seem to be up to date with latest master. This commit re-introduces several changes made in the past few weeks, such as re-separation of |
|
@RonConescu I assume you're going to update this before we review? I emailed Pablo earlier, did that get through? |
|
@jwe-apple Your comments were received and this PR will be updated. |
This commit contains a major architectural revision: cleaning up our scheduling system. The original system scheduled tasks into the future; this system maintains the scheduling rules, but doesn't actually schedule anything until the user taps on it. We also: - centralized lots of code, making it easier to maintain and debug - made certain common features (schedule delays and task-expiration periods) data-driven instead of software-driven This commit addresses concerns raised in the pull-request comments to my previous commit: - DataSubstrate has been re-consolidated (and extended) - superfluous files have been removed - formatting has (mostly) been preserved
|
Hi, folks. I've submitted a second commit. |
|
@jwe-apple , @YuanZhu-apple , @p2 -- I completely redid the contents of the pull request, and updated the introductory comment. I've also submitted a bunch of subsequent edits to comments and formatting. Since I haven't seen any comments from you, I'll assume that GitHub hasn't sent you an email about any of that... ? I look forward to your next round of feedback. |
| { | ||
| [self finishRefreshCommandPassingError: errorFromServerFetch | ||
| toCompletionBlock: completionBlock]; | ||
| } |
There was a problem hiding this comment.
Declare a weakSelf outside this block and use it here to avoid reference loop.
__weak typeof(self) weakSelf = self;
|
Sorry for the delay @RonConescu . I did not test the code, but from going over it in GitHub this looks all good from my side – I'm mostly focused on keeping interdependencies down, and there are improvements on that front in this PR. (One hint: coding-style-guide.md) |
|
|
||
| @see -dateByAddingISO8601Duration: | ||
| */ | ||
| - (NSDate *) dateBySubtractingISO8601Duration: (NSString *) durationString; | ||
|
|
There was a problem hiding this comment.
Generally, I think all these utility function need test coverage.
There was a problem hiding this comment.
Created a GitHub issue for this.
|
John, Pascal, Yuan, Thank you for the comments above. A general question: for many of the pull requests on this project, you typically have many comments and requirements, both granular and architectural. Pascal and Yuan's comments above seem more granular than architectural, which suggested to me that I should wait to make more changes until you'd had a chance to add architectural requirements. (Obviously, Pascal's "interdependencies" comment is architectural, but it was also a "looks good" instead of a "please change such-and-such.") Should I continue to wait? If I implement Pascal and Yuan's stuff, above, will this pull request become acceptable? Thanks, very much. Ron (to/cc: @jwe-apple @p2 @YuanZhu-apple @insha @peculiar @pabloarce-yml) |
|
Gang, I've submitted a new commit, with responses to each item above. I look forward to your feedback. Ron cc: @jwe-apple @p2 @YuanZhu-apple @insha @peculiar @pabloarce-yml @RonConescu |
|
I'm not set up to test the new scheduler just yet, hence no substantial comments from my side and no objections either. |
| if (serializedTimesOfDayString.length > 0) | ||
| { | ||
| NSDateFormatter *formatter = [NSDateFormatter new]; | ||
| formatter.locale = [NSLocale localeWithLocaleIdentifier: @"en_US_POSIX"]; |
There was a problem hiding this comment.
Use kAPCDateFormatLocaleEN_US_POSIX
There was a problem hiding this comment.
Done. Also applied that constant to everywhere else in AppCore which needed it.
|
@RonConescu I think this is ready to go after some response for the last comment I made. |
|
@YuanZhu-apple — Back to you, sir. |
New task-scheduling infrastructure.
NSDate+Helper addition
Take 2.
This commit contains a major architectural revision: cleaning up our scheduling system. The original system scheduled tasks into the future; this system maintains the scheduling rules, but doesn't actually schedule anything until the user taps on it.
We also:
This commit addresses concerns raised in the comments to my previous commit: