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

Top level function for migrations #66

Merged
merged 1 commit into from
Apr 2, 2016
Merged

Top level function for migrations #66

merged 1 commit into from
Apr 2, 2016

Conversation

marius-serban
Copy link
Contributor

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I have resolved any merge conflicts.
  • I have rebased on develop squashed my commits into 1 commit.
  • I have followed the coding style, and reviewed the contributing guidelines.

This fixes issue #46 .

What's in this pull request?

Added a top level function that performs progressive migrations.
The function is a bit large, it could be broken down but not sure what's your preference @jessesquires .

}

return Array(modelDirectoryURLs.flatMap { $0.lastPathComponent }.flatMap { bundle.URLsForResourcesWithExtension("mom", subdirectory: $0) }.flatten())
.flatMap { NSManagedObjectModel(contentsOfURL: $0) }
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 breakup this return into multiple steps with descriptive variable names so its easier to read 😄

@jessesquires
Copy link
Owner

Thanks @marius-serban ! 😄 🎉

At first glance, this looks great! Before I review in-depth, let's do some re-organization:

  • Move new code from CoreDataExtensions.swift to a new file, Migrate.swift
  • Pull the inner functions out, try to breakup the remaining parts of the function into smaller ones
  • All helper functions should be internal and tested if possible/if it makes sense, otherwise let's make them private

@jessesquires
Copy link
Owner

@marius-serban - Is this ready to go? 😄

@jessesquires jessesquires added this to the 4.0.0 milestone Mar 28, 2016
@marius-serban
Copy link
Contributor Author

yup! made those changes

Marius

On 28 Mar 2016, at 04:23, Jesse Squires notifications@github.com wrote:

@marius-serban - Is this ready to go?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@jessesquires jessesquires modified the milestones: 3.0.0, 4.0.0 Apr 2, 2016
@jessesquires
Copy link
Owner

🎉 Thanks so much @marius-serban !

I'll be merging soon. here's the plan:

  1. Release 2.2.1
  2. Merge this and release 3.0 with Swift 2.2

@jessesquires jessesquires merged commit 68a1fca into jessesquires:develop Apr 2, 2016
@jessesquires
Copy link
Owner

@marius-serban - just hand another look at this after merging. And I'm like:

Please keep contributing! 😄

@jessesquires
Copy link
Owner

hey @marius-serban !

i'm going through PRs to add contributors as collaborators on this project. just sent you an invite! 😄

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.

None yet

2 participants