Skip to content

Conversation

@koke
Copy link
Member

@koke koke commented Feb 3, 2016

It all starts when I try to add nullable to planID so I can properly use it in Swift. If you add nullability for one attribute, you have to do the whole file. Since I had to go through every attribute on Blog and figure out if it was nullable or not, I thought I might as well extract them all, convert them to Swift, and add documentation.

I'm going to test and investigate this for a bit, but @astralbodies maybe you already know the answers to these questions:

  • Can use the typed Set instead of NSSet for core data relationships?
  • If a to-many relationship is optional, does that mean it should be NSSet?, or it will always be an instance of NSSet (that might be empty)?

I've tried to change as little as possible on this PR, but already found some interesting next steps:

  • DiscussionSettingsViewController should be able to handle blog.settings being nil. I hate to commit a settings!, but it's no different than what was there before.
  • There are a bunch of unused attributes that should be removed from the data model.
  • There are also a few that don't seem to be set anywhere but were added recently, so I assume they're work-in-progress feature (menus, tags)
  • Blog.options is a mess. We should at least stop using the legacy structure. Ideally we'd move everything into BlogSettings, or turn some things into Blog properties.
  • Migrate blogID to dotComID. blogID can either be the wp.com ID or nil, so we should rename it to avoid confusion.
  • Simplify account/jetpackAccount. As I was writing the documentation I felt the urge to stop and refactor that. It's just terribly confusing, and there should be just Blog.account

Needs Review: @astralbodies

@koke
Copy link
Member Author

koke commented Feb 3, 2016

If a to-many relationship is optional, does that mean it should be NSSet?, or it will always be an instance of NSSet (that might be empty)?

(lldb) po [[blog menus] class] _NSFaultingMutableOrderedSet

Seems like they're always set, but I'm not sure if that's guaranteed.

@koke
Copy link
Member Author

koke commented Feb 3, 2016

Actually, I checked the generated classes, and it's all optionals, which makes a lot of sense. You don't pass the attributes on initialization, so they all can be nil. Optional checking is done when saving.

However, I tested on a newly created Blog and it seems Core Data is "smart" about this:

(lldb) po blog.valueForKey("xmlrpc")
nil

(lldb) po blog.xmlrpc
""

This means no crashing, but I wonder if this brings any surprising side effects.

@koke koke added this to the 6.1 milestone Feb 3, 2016
@koke koke mentioned this pull request Feb 4, 2016
@astralbodies astralbodies self-assigned this Feb 8, 2016
@astralbodies
Copy link
Contributor

Excellent work! :shipit: after merging in develop 😄

@koke
Copy link
Member Author

koke commented Feb 9, 2016

I don't think this is ready to merge yet. I've been writing some tests, and this is a bit unexpected:

(lldb) po blog.blogID as Int
0

(lldb) po blog.blogID as Int?
▿ Optional(0)
  - Some : 0

(lldb) po blog.blogID as NSNumber
 <uninitialized>
(lldb) po blog.blogID as NSNumber?
nil
(lldb) po blog.blogID as NSNumber!
fatal error: unexpectedly found nil while unwrapping an Optional value

I'm tempted to use the implicitly unwrapped optional version, but I'm collecting my thoughts and considering that this is just a symptom of a bigger issue, and we should just stop passing NSManagedObjects all over the place.

These tests verify what I've found so far about mapping core data attributes to
Swift types.
@astralbodies
Copy link
Contributor

we should just stop passing NSManagedObjects all over the place

Yup. This. We really need to put Core Data down as low as possible and use a separate data model (transfer object?) for what we use in the UI. The problem comes with fetched results controllers so we'd need to solve those performance issues if we wanted to not mix UI with CD.

@astralbodies
Copy link
Contributor

Regarding Sets and relationships - sorry I missed that piece.

NSSet to Set I believe is okay.

Relationships (and pretty much all accessors) seem to be optional when generated by Xcode regardless of the optionality of the attribute in the Core Data model. This makes Core Data validation more important to handle - which we don't really do anywhere since it sucks.

@astralbodies
Copy link
Contributor

@koke Let me know when you want to take a look at this one again!

@koke
Copy link
Member Author

koke commented Feb 18, 2016

I'm tempted to close this for now as I don't really know how to handle the optionals, short of implementing an immutable non-managed version of Blog

@astralbodies
Copy link
Contributor

We can close the PR for now - easy enough to reopen when we want to figure something out better.

@astralbodies astralbodies modified the milestones: Someday, 6.1 Feb 19, 2016
@astralbodies astralbodies deleted the feature/blog-core-data-properties branch April 20, 2016 12:30
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