Allow property assignments to pull values from functions #1545
Conversation
theunrepentantgeek
requested review from
babbageclunk,
davefellows,
matthchr and
Porges
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1545 +/- ##
==========================================
+ Coverage 62.17% 63.59% +1.41%
==========================================
Files 163 178 +15
Lines 10739 11726 +987
==========================================
+ Hits 6677 7457 +780
- Misses 3422 3606 +184
- Partials 640 663 +23
Continue to review full report at Codecov.
|
|
|
||
| // ReturnTypes returns nil as we can't represent byte[] in our type system | ||
| func (f *OneOfJSONMarshalFunction) ReturnTypes() []Type { | ||
| return nil |
There was a problem hiding this comment.
Presumably this isn't a problem because ReturnTypes is just used for References?
There was a problem hiding this comment.
I've removed this method - have added a new interface ValueFunction which is implemented only where needed, eliminating the need to add ReturnTypes() to a bunch of existing functions.
| conv, err = fn.createPropertyConversion(receiverProperty, otherProperty) | ||
| default: | ||
| panic(fmt.Sprintf("unexpected conversion direction %q", fn.direction)) | ||
| // Generate a conversion from one property to another | ||
| conv, err := fn.createPropertyConversion(sourceProperty, destinationProperty) | ||
| if err != nil { | ||
| // An error was returned, we abort creating conversions for this object | ||
| return errors.Wrapf( | ||
| err, | ||
| "creating conversion setting property %q from property %q", | ||
| destinationProperty.PropertyName(), | ||
| sourceProperty.PropertyName()) | ||
| } else if conv != nil { | ||
| // A conversion was created, keep it for later | ||
| fn.conversions[destinationName] = conv | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| sourceFunction, ok := sourceFunctions[destinationName] | ||
| if ok { | ||
| // Generate a conversion to assign the property from the function | ||
| conv, err := fn.createFunctionConversion(sourceFunction, destinationProperty) | ||
| if err != nil { | ||
| // An error was returned, we abort creating conversions for this object | ||
| return errors.Wrapf(err, "creating conversion for property %q of %q", receiverProperty.PropertyName(), receiver.Name()) | ||
| return errors.Wrapf( | ||
| err, | ||
| "creating conversion setting property %q from function %s()", | ||
| destinationName, | ||
| sourceFunction.Name()) |
There was a problem hiding this comment.
These two chunks (and the createPropertyConversion and createFunctionConversion methods below) are very similar. Would it be feasible to make wrappers for sourceProperty and sourceFunction to make them expose the same interface (where one reads from a selector and the other reads from a callexpr) and then use that in a single createPropertyConversion method?
There was a problem hiding this comment.
I've created two wrapper end points - ReadableConversionEndpoint and WritableConversionEndpoint that achieve a nice reduction in duplication.
theunrepentantgeek
force-pushed
the
feature/function-return-types
branch
2 times, most recently
from
a1db005 to
ca35fe9
Compare
theunrepentantgeek
force-pushed
the
feature/function-return-types
branch
from
ca35fe9 to
ee8ea72
Compare
theunrepentantgeek
force-pushed
the
feature/function-return-types
branch
from
ee8ea72 to
e436b52
Compare
| "creating conversion to %s by %s", | ||
| sourceEndpoint, | ||
| destinationEndpoint) |
There was a problem hiding this comment.
Are the arguments here the right way around?
There was a problem hiding this comment.
They need to be swapped; will do that in my next PR.
What this PR does / why we need it:
Allows generated conversions to read a value from a suitably named source function for writing into a field on the destination type. Having this will allow resolution of #1468 by having a function on an API variant (e.g.
v1alpha1api20210601.Person) that is used to populate a field on the associated storage variant (e.g.v1alpha1api20210601storage.Person).Special notes for your reviewer:
Implementing a generic solution was cleaner than trying to do lots of semi-nasty special casing. Introducing
ValueFunctionavoided some wide spread changes to existing implementations ofFunction.There's also a little tidy up around the golden tests as they were previously using private (lowercase) field names when our generated types actually use public (CapitalCase) naming.
How does this PR make you feel:
If applicable: