fluent-kit icon indicating copy to clipboard operation
fluent-kit copied to clipboard

Set method for group fields + tests

Open JeronimoPaganini opened this issue 2 years ago • 4 comments

Since in current version I didn't find how to use similar way to setting values for fields marked like Group, I propose my small improvement.

Example:

try Galaxy
    .query(on: self.database)
    .set(\.$size.$km, to: oneLightYearInKm)
    .set(\.$size.$lightYear, to: 1)
    .update()

where $size is Group fields in a model

    @Group(key: "area")
    public var size: Size
public final class Size: Fields {
    @Field(key: "km")
    var km: Double
    
    @Field(key: "light_year")
    var lightYear: Double
    
    public init() { }
}

If I misunderstood something, please, let me know :)

JeronimoPaganini avatar Sep 14 '21 15:09 JeronimoPaganini

@Joannis I need to look at this implementation more closely in any event, but am interested in your input, specifically re: the test failures for Mongo integration where we're apparently hitting the "didn't fetch or set before accessing" codepath?

gwynne avatar Sep 16 '21 07:09 gwynne

Hi @gwynne , I guess you meant @JeronimoPaganini , I'm a bit confused how it happened, since I've reused the same method set, and I've added just +1 signature here: https://github.com/vapor/fluent-kit/pull/454/files#diff-060bb4b838cade73b63ca17a0ce5a59743bcc45b4ba559f4673bb3a6f79b9726R40

So the plan was just supporting group kodepath (I've re-checked it in my project using Postgres, but didn't check how it works with Mongo).

Could it be, since mongo-kit doesn't work with group codepaths? (Sorry for the stupid question, I didn't work with mongo-kit yet)

JeronimoPaganini avatar Sep 16 '21 07:09 JeronimoPaganini

@JeronimoPaganini no, she meant me. I'm maintaining all MongoDB related components.

Joannis avatar Sep 16 '21 07:09 Joannis

@Joannis, Ah, got it. We have the same 1st letter nickname "J", and since I've prepared this PR, I thought it's just T9 :)

JeronimoPaganini avatar Sep 16 '21 07:09 JeronimoPaganini