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

optional relations #62

Closed
tanner0101 opened this issue Aug 26, 2019 · 4 comments
Closed

optional relations #62

tanner0101 opened this issue Aug 26, 2019 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@tanner0101
Copy link
Member

tanner0101 commented Aug 26, 2019

@Parent, @Children, and @Siblings should support optional relations.

Some ways to achieve this are:
1: Make all relations optional
2: Make all relations optional and provide @Required* versions, i.e., @RequiredParent
3: Make all relations required and provide @Optional* versions, i.e., @OptionalParent
4: Allow relations to be declared as optional / required on individual basis, i.e.:

Note: It's unclear whether this one would be easy to do with property wrappers, but it has a very nice API

// optional relation
@Parent(...)
var galaxy: Galaxy?

// required relation
@Parent(...)
var galaxy: Galaxy
@tanner0101 tanner0101 added the enhancement New feature or request label Aug 26, 2019
@jdmcd
Copy link
Member

jdmcd commented Aug 26, 2019

I think that all relations should be required by default, with the ability to specify an optional variant. I do not think that making all relations optional or even requiring something like RequiredParent would be good for beginners, readability, or maintainability. I have no preference as to whether it's @Parent with an optional property or @OptionalParent.

@PopFlamingo
Copy link

If we can't go with solution number four, I agree with @mcdappdev: it would be better to avoid @Required* as I feel it would make the syntax "heavier" for what is probably going to be the most commonly used type of relation.

@tanner0101 When you say it's not clear wether or not 4) is makable, what are the main issues you are seeing? From what I just tried it's not possible to overload the property warper init so it seems like a dynamic cast is required, is this about performance you are worried?

@tanner0101
Copy link
Member Author

tanner0101 commented Aug 27, 2019

@adtrevor I was thinking something like this:

@propertyWrapper
final class Parent<Value> {
    var _id: Any

    init(key: String) { ... }
}

extension Parent where Value: Model {
    var id: Value.ID {
        ...
    }

    func get(on database: Database) -> EventLoopFuture<Value> {
        ...
    }
}

extension Parent where Value: OptionalType, Value.Wrapped: Model {
    var id: Value.Wrapped.ID {
        ...
    }

    func get(on database: Database) -> EventLoopFuture<Value.Wrapped?> {
        ...
    }
}

The idea here is that the property wrappers don't have a hard requirement on the Value being a model. Instead, the property wrapper holds some type-erased properties (like _id: Any) and then those properties are interpreted in extensions that know exactly what type the model is.

That way @Parent var galaxy: Galaxy and @Parent var galaxy: Galaxy? can have different storage and different methods. This is useful because the required relation's get method should not return an optional. If it can't get the model for some reason, that should be an error. The second relation's get method should return an optional though.

However, as you said, this could get weird with init method overloads and stuff. I can't be sure whether or not this would work until I or someone else tries it.

@tanner0101
Copy link
Member Author

@OptionalParent was added a while back. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants