-
Notifications
You must be signed in to change notification settings - Fork 395
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
RFC: Swift 2.0 Changes #302
Comments
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. |
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. |
Ugh. They still haven't fixed that? Welp. Scratch the all scalar all the time thing. |
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. |
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. |
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. |
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! |
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. |
@brentdax I pushed a few changes:
I am not sure how to handle entityName() as a computed property in the instance that someone subclasses? ie. 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! |
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. |
@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 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 var type: Int16 {
get {
willAccessValueForKey(Attributes.type.rawValue)
defer { didAccessValueForKey(Attributes.type.rawValue) }
return (primitiveType?.shortValue)!
} The force unwrap in the |
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 :-). |
@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.
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: |
@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 |
@protocool The reason is to prevent |
@jdriscoll understood. In this case 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. |
What is the rationale for using an implicitly unwrapped Optional in this init? Why not use |
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. |
ps. reverted back to |
@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 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 |
@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. |
any update on swift template? Is it possible to use the template used for mogenerator-sample ? |
Not presently. I work on it as I have time. |
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. |
@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 |
@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? |
Hi! Any сhanges / releases? Scalars? Swift extension? |
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.
EntityName+MOGenerator.swift
is an extension on your managed object subclass. This is a replacement for_EntityName.swift
.MOGenerator
protocol that hasentityName()
andentity
class functions define. Extending thatMOGenerator
protocol implements those methods, rather than cloning them all in each subclass.NSNumber
to specific scalar values:Int
,Int32
, etc.validate*
methods to usetry/catch
.NSSet
,Set<NSObject>
to be more sane.Feedback welcomed on this. Nothing is set in stone yet.
The text was updated successfully, but these errors were encountered: