This repository has been archived by the owner on Feb 29, 2020. It is now read-only.
[iOS] Returning operations from sync actions #709
Merged
shrishrirang
merged 9 commits into
Azure:dev
from
damienpontifex:dev-ReturningOperations
May 26, 2015
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
920e311
returning operations types on sync methods
damienpontifex f53a02e
testing pull operation being returned appropriately
damienpontifex 4249495
test push operations returning appropriately
damienpontifex 58c74d3
test purge operations returning appropriately
damienpontifex ec0da03
Feedback from pull request and only returning NSOperation instances t…
damienpontifex 9a4eaad
updated docs to describe push operation prior to pull if pending oper…
damienpontifex ee7f589
feedback from PR
damienpontifex 6ffcfdc
further formatting fixes converting tabs to spaces for indentation
damienpontifex 50d9ccb
marking pull operation as complete if cancelled plus unit tests for case
damienpontifex File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are you sure calling cancel is enough? Doesn't that just set the isCancelled flag? The op itself wouldn't set isFinished until it goes to run. What flag is the dependent Op observing?
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.
Cancelling the NSOperation would mark it finished, so the observing task should be fine.
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.
The operation wouldn't have even started at this point, so I don't think we need to do anything else. The completion block will be called if the user has provided one to notify them of the failure.
I could do some testing though with the returned pull operation as a dependency of another operation. If the execution flows through to the waiting operation when this is cancelled, should all be fine.
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 you link to docs? Everything I see online, says that the behavior of cancel isn't different if the operation is not running. So if you want to trigger a dependent task, you are responsible for setting the isFinished flag as well.
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.
Running through a unit test, you were right @phvannor that without explicitly setting the finished status, the dependent tasks wouldn't be completed.
Latest commit with changes to access internal method for completion plus unit test for this case added.
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.
Thanks for adding UT to test this path.
Btw, should we not simply add the pull NSOperation to the queue immediately after making it dependent on the push NSOperation (instead of waiting for the push to complete successfully)? That way you won't need to call 'completeOperation' explicitly. Just calling cancel ( [push cancel] ) will be enough.
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.
Sadly, then the pull would possibly already be running, so we'd actually need the push op to expose its error state, so the pull op could look at its dependencies & verify they are were ok before running. Probably worth looking into, but not sure on effort to do so vs what we have now.
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.
Right!