rentzsch / mogenerator

Core Data code generation
http://rentzsch.github.com/mogenerator
MIT License
3.03k stars 395 forks source link

RFC: Swift 2.0 Changes #302

Open justin opened 9 years ago

justin commented 9 years ago

We are discussing changes to make for Swift 2.0. Given that Apple has had no qualms about breaking support for Swift between releases, we want to use it as an opportunity to modernize our Swift support to take full advantage of the language.

This won't have any effect on the Objective-C code, but if you're using our Swift support, you will likely need to massage your code a bit.

Feedback welcomed on this. Nothing is set in stone yet.

rentzsch commented 9 years ago

At first take all of this looks great. Swift extensions can Do More than Obj-C categories, so I think they're a win now.

jdriscoll commented 9 years ago

How do you plan to handle optional attributes? While scalar values can be optional in Swift you can't currently mark an @NSManaged scalar property as optional. Hopefully this will change but as of Xcode 7 b6 the compiler will complain it can't be represented in Objective-C. A nil value for an optional Integer 64 will return zero if defined as Int64 in your model.

justin commented 9 years ago

Ugh. They still haven't fixed that? Welp. Scratch the all scalar all the time thing.

justin commented 9 years ago

Pull request #305 is sort of starting us down this path. Need to address it before I can move on to knocking out some Swift-ey stuff.

frr149 commented 8 years ago

What's the time frame for making the generated code Swift 2.0 compliant? If nobody is currently working on this, I'll jump in.

justin commented 8 years ago

Here's a sample project that is based on where the default Swift templates are heading. I'm using scalar types where possible thanks to a blog post by @protocool.

https://github.com/justin/mogenerator-sample

Feedback welcomed. The next step is to convert this format into templates and then update the internals of mogenerator to generate code in this style, while still keeping things sane for Objective-C users.

beccadax commented 8 years ago

Thanks for asking me to chime in. A few design comments:

And implementation things:

If I haven't talked about it, that means I like it. This is some great work!

rentzsch commented 8 years ago

I took a look at https://github.com/justin/mogenerator-sample. Looks Good To Me. I'm still not using Swift in anger, so I'm not the best judge on the Swift codegen side of mogenerator. I'm very happy to delegate control of Swift codegen to @justin, who's actually using it to Get Work Done.

@brentdax thanks for the insightful feedback.

justin commented 8 years ago

@brentdax I pushed a few changes:

  1. Use Int instead of int16|32|64
  2. Rename MOGeneratorEntity to MOGeneratorEntityType
  3. Move Attributes & Relationships Enums internal to classes.
  4. Move entity(managedObjectContext:) to protocol extension so we can DRY up our models.

I am not sure how to handle entityName() as a computed property in the instance that someone subclasses? ie. ParentMO and ChildMO are subclasses of HumanMO. Ideas?

Not really following the thing re the implicitly unwrapped options in the getters. Small code snippet example?

And re: the overloads, I'll leave that as an exercise for a future release. I just want to get a decent foundation for Swift in here before we go forward.

Thanks for the feedback!

jdriscoll commented 8 years ago

I'm opposed to using Int. Int is either 32 bit or 64 bit depending on platform and Core Data will overflow if given a larger value than it expects. If you're assuming 64 bit numbers your app will be broken on 32 bit devices. If you're looking to store 16 bit numbers your in memory storage will not reflect what will be saved to disk.

beccadax commented 8 years ago

@jdriscoll I was hoping that the object would fail to validate if you tried to store a number that wasn't in the type's range, but I just did a bit of playground testing and unfortunately Core Data doesn't seem to do that. So as much as I hate it, we may need to switch back to using specifically-sized types, or at least provide some kind of option to do so.

@justin I believe that you can generate the child class's entityName property with an override keyword. Or am I wrong about that? Maybe there's some sort of complication I didn't anticipate.

On the overloads: I think it's okay to push them off, but only if we really do want to implement them later. If we don't, we should just switch to String constants now; they're in practice more useful if we're not going to offer overloads. (By the way, I wouldn't mind helping to implement the overloading and other type systems surrounding this.)

What I'm worried about isn't implicitly unwrapped optionals; it's the forced unwrapping in the getters. For example, consider this getter in ChildMO:

    var type: Int16 {
        get {
            willAccessValueForKey(Attributes.type.rawValue)
            defer { didAccessValueForKey(Attributes.type.rawValue) }
            return (primitiveType?.shortValue)!
        }

The force unwrap in the return line means that, if you call this getter before you initialize type (or primitiveType), it will crash. Now, in theory, that's perfectly appropriate behavior—you're trying to access something you haven't initialized. But in practice, I suspect it will cause trouble. In particular, I've seen hints on the swift-evolution list that, in the future, they will not provide direct access to the setter as a function, but will instead offer an (inout T -> Void) -> Void "lens" which can modify it in-place. Like any inout-based function, this would mean that it would call the getter, pass in the value, and then call the setter with the modified value—and if the getter force-unwraps an optional, that could be trouble.

groue commented 8 years ago

In particular, I've seen hints on the swift-evolution list that...

If I were you, I would get much more specific, or I would stop worrying. In both cases, don't hesitate: share the link to the swift-evolution thread, and be vocal about your concern for mogenerator. Or write your getter in a way that works now, regardless of the possible futures of Swift. I guess the latter solution is the one that is the most wanted by mogenerator users :-).

protocool commented 8 years ago

@brentdax / @justin the custom accessor for ChildMO.type is unnecessary (and, as written, incorrect) if it's simply declaring a scalar type that's compatible with Objective-C.

@NSManaged var type: Int16 will behave the same as @property (assign) int16_t type;, transparently converting nil primitive values to 0.

Obviously nil or garbage primitive values can still be an issue for other property types, such as enums. I didn't specifically talk about it in my article, but in those cases I generate getters that fallback to a meaningful hard-coded value, as in: return primitiveSomething.flatMap { Something(rawValue: $0.integerValue) } ?? .AnAppropriateDefault

beccadax commented 8 years ago

@groue There's no formal proposal, simply some rumblings from the core team. This post references the idea a little bit obliquely.

@protocool If the @NSManaged approach works, we should use it. I assumed there was some reason we were defining our own.

jdriscoll commented 8 years ago

@protocool The reason is to prevent nil being converted to 0 and allowing for optional scalar properties on your managed objects. This is needed when a value can either be null OR 0 and both are valid.

protocool commented 8 years ago

@jdriscoll understood. In this case ChildMO.type is declared to Swift as an Int16 so a null primitive value must either be treated as exceptional or coerced to a reasonable representation of null for the type.

Calling the current implementation 'incorrect' was a bit of an overstatement. It's more that it is inconsistent with the default generated scalar accessor, which coerces to 0.

frr149 commented 8 years ago

What is the rationale for using an implicitly unwrapped Optional in this init? init(entity: NSEntityDescription, insertIntoManagedObjectContext context: NSManagedObjectContext!)

Why not use NSManagedObjectContext?

justin commented 8 years ago
    var type: Int16 {
        get {
            willAccessValueForKey(Attributes.type.rawValue)
            defer { didAccessValueForKey(Attributes.type.rawValue) }

            if let t = primitiveType {
                return t.shortValue
            }

            return -666
        }
        set {
            willChangeValueForKey(Attributes.type.rawValue)
            defer { didChangeValueForKey(Attributes.type.rawValue) }
            primitiveType = NSNumber(short: newValue)
        }
    }
    @NSManaged private var primitiveType: NSNumber?

In the case of getting around the implicitly unwrapped options, I'm honestly not sure what to put for the return in the extreme case that you don't get a value back. Nothing really stands out as the proper solution to me. Feedback welcomed.

justin commented 8 years ago

ps. reverted back to Int16 and friends. https://github.com/justin/mogenerator-sample/commit/be20d3041c52eb3f41395fd95a2c947992502840

protocool commented 8 years ago

@justin are you specifically trying to solve the problem of non-optional scalar properties here or is this just your example of dealing with the general case of nil primitive values when the property is non-optional?

Because if you're actually trying to solve the issue of a non-optional Int16 (and friends), then Core Data already handles that: declare the property as @NSManaged var type: Int16 and any nil primitiveType is automatically coerced to 0 by the accessor that Core Data generates.

If you're trying to construct a general-case solution for non-optional, non-scalar properties which cannot be expressed in Objective-C, such as an enum ThingType: Int16, then you're already relying on 'custom' factory code, so IMO it's reasonable to enforce a rule which says that if you supply a default value in the attribute's userInfo, it will be used in a flatMap-or (which I described in https://github.com/rentzsch/mogenerator/issues/302#issuecomment-168602807 a few weeks back), otherwise the value will be implicitly unwrapped.

protocool commented 8 years ago

@frr149 WRT your question about the signature for the init (and in case you didn't come across the answer yourself), that's the signature for the designated initializer of NSManagedObject.

akabab commented 8 years ago

any update on swift template? Is it possible to use the template used for mogenerator-sample ?

justin commented 8 years ago

Not presently. I work on it as I have time.

pip8786 commented 8 years ago

Is there a way for Swift generation to use Set instead of NSSet? If not, would it be useful for me to try and contribute that (if it's possible)? As of right now I just changed it in my machine files and turned off auto generation on every build.

rentzsch commented 8 years ago

@pip8786 I don't know the tradeoffs, we'll see what @justin has to say.

In terms of your own templates, it's really easy to use your own variant that uses Set instead NSSet. Just download the files in mogenerator's /template git dir, hack them up to your liking and then pass --template-path /path/to/templates-hacked/ to mogenerator.

pip8786 commented 8 years ago

@rentzsch Thanks for the tip on using templates.

I realize now that I didn't even mention that the big reason for wanting this was to have generics as well so that I don't have to cast anything coming out of the Set. Looking through the code it looks like they're left out specifically for Swift code generation (line 591 of mogenerator.m). Was there a reason for that?

Igor-Palaguta commented 8 years ago

Guys, as an experiment added MotoSwift

It allows to specify file name mask It uses simple Stencil as template language It uses scalar types Allows to customize templates with Entity/Attribute/Relationship/FetchedProperty userInfo

makleso6 commented 7 years ago

Hi! Any сhanges / releases? Scalars? Swift extension?