Add support for discontinuous property conversions #3641
Conversation
theunrepentantgeek
requested review from
davefellows,
matthchr,
babbageclunk and
super-harsh
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3641 +/- ##
==========================================
- Coverage 53.58% 53.45% -0.14%
==========================================
Files 1411 1417 +6
Lines 483451 484060 +609
==========================================
- Hits 259079 258731 -348
- Misses 185153 186094 +941
- Partials 39219 39235 +16 ☔ View full report in Codecov by Sentry. |
theunrepentantgeek
force-pushed
the
fix/discontinuous-object-conversion
branch
from
1671f7b to
1fca1b7
Compare
| // Require a path from one name to the next, and work out an intermediate step to break down the conversion | ||
| var intermediateName astmodel.InternalTypeName | ||
| if conversionContext.PathExists(sourceName, destinationName) { | ||
| var err error |
There was a problem hiding this comment.
Can move this(and below) var outside the if statements and to the error checking after the if-else blocks?
There was a problem hiding this comment.
We could, but I prefer keeping things tightly scoped.
We need intermediateName after the if, but we don't need either of the potential errors later on. Moving the err variable outside implies a need to access the error again, which isn't correct.
|
|
||
| next, err := graph.FindNextType(current, nil) | ||
| if err != nil { | ||
| break |
There was a problem hiding this comment.
Should we be doing something with this error?
There was a problem hiding this comment.
If there's an actual problem (other than no next type) that'll be reported elsewhere. So I think this one is safe to ignore.
| c.start, | ||
| func(t astmodel.InternalTypeName) bool { | ||
| count++ | ||
| return t == c.end |
There was a problem hiding this comment.
Should we be using .Equals here?
There was a problem hiding this comment.
I don't think we need to - we're comparing InternalTypeName instances, which are value types, not reference - and we don't need any of the special support from .Equals.
| } | ||
|
|
||
| if intermediateName.IsEmpty() || astmodel.TypeEquals(intermediateName, sourceName) { | ||
| if intermediateName.IsEmpty() || | ||
| astmodel.TypeEquals(intermediateName, sourceName) || |
There was a problem hiding this comment.
minor: Should there be a comment here explaining the situations where the intermediate name is the source or destination?
| // | ||
| // Note the actual steps are generated by nested conversions; this handler works by finding the two conversions needed | ||
| // given our intermediate type and chaining them together. | ||
| func assignNoninlineObjectsViaPivotObject( |
There was a problem hiding this comment.
minor, should this be:
| func assignNoninlineObjectsViaPivotObject( | |
| func assignNonInlineObjectsViaPivotObject( |
Since I think non-inline would be cased like that?
What this PR does / why we need it:
We've had the assumption that two properties needing conversion would have line-of-sight to each other in the storage conversion graph constructed by the generator. To this point this has been true, but with a recent attempt to import another preview version of
ManagedClusterwe discovered that this was not so.To support these conversions, I've added another conversion handler, one that looks for a commonly visible pivot type that can be used to build the conversion required.
Special notes for your reviewer:
I've also refactored creation of test cases for property conversions - the existing test case factory method was too large and complicated to comprehend; now the various groups of test cases are split up.
How does this PR make you feel:
If applicable: