Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Issue 60 stack structure #72

Merged
merged 2 commits into from
Apr 26, 2016
Merged

Issue 60 stack structure #72

merged 2 commits into from
Apr 26, 2016

Conversation

Liquidsoul
Copy link
Collaborator

Pull request checklist

This fixes issue #60

What's in this pull request?

  • change stack structure so that both background and main contexts are connected to the persistent store coordinator
  • sync mechanism between contexts now use mergeChangesFromContextDidSaveNotification(_:)

What's not in this pull request?

  • child contexts creation with background context as parent.
    childContext use the main context as parent. This may not be something we want with this new structure (at least not all the time).
  • some of the tests may be simplified or changed to reflect the new stack structure and update mechanism.


Data between these two primary contexts and child contexts is kept in sync.
Changes to a context are propagated to its parent context and eventually the persistent store when saving.
Data between these two primary contexts is kept in sync using `NSManagedObjectContextDidSaveNotification` and `mergeChangesFromContextDidSaveNotification(_:)`.
Copy link
Owner

Choose a reason for hiding this comment

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

let's change this to:

Data between the main and background contexts is perpetually kept in sync.

@jessesquires
Copy link
Owner

jessesquires commented Apr 26, 2016

Whoa @Liquidsoul ! This is awesome! 👍 😄 Great work. Perfect pull request. ✨

Only one small comment to address, then I'll merge. Thanks so much for working on this!

I'll be releasing this with 4.0.0 soon.

@jessesquires
Copy link
Owner

What's not in this pull request?

  • child contexts creation with background context as parent. childContext use the main context as parent. This may not be something we want with this new structure (at least not all the time).

Yep, agreed. I created #73 to follow up here. 😄

  • some of the tests may be simplified or changed to reflect the new stack structure and update mechanism.

I think what you've done with the tests is great! 👍 Is there something specific you think we should change?

mainContext is now directly connected to the persistent store
coordinator instead of being a child of the background context.
Changes are merged between background and main contexts using
`mergeChangesFromContextDidSaveNotification(_:)`
@Liquidsoul
Copy link
Collaborator Author

Thanks @jessesquires ! 😊

I've amended my last commit to change the documentation and rebased the branch.

I think what you've done with the tests is great! 👍 Is there something specific you think we should change?

I was wondering if the sibling context updates checks in SaveTests.swift could be removed.
We test that saving the background and the main context will merge the changes into the other, it may not be necessary to do this kind of check in all the tests.
Maybe this could reduce the size of some of those tests without reducing the coverage.

@jessesquires
Copy link
Owner

@Liquidsoul ahhh, I see. I actually like this. Feels very thorough. 😄

@jessesquires jessesquires merged commit 1c9574e into jessesquires:develop Apr 26, 2016
@Liquidsoul Liquidsoul deleted the issue_60_stack_structure branch April 26, 2016 18:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants