Support wildcards with transformation targets #2325
Conversation
Prerequisite for allowing wildcards
theunrepentantgeek
requested review from
davefellows,
Porges,
matthchr,
babbageclunk and
super-harsh
as code owners
| @@ -16,6 +16,8 @@ type StringMatcher interface { | |||
| Matches(value string) bool | |||
| // WasMatched returns nil if the matcher had a match, otherwise returning a diagnostic error | |||
| WasMatched() error | |||
| // IsRestrictive returns true if the matcher is populated and will restrict matches | |||
| IsRestrictive() bool | |||
There was a problem hiding this comment.
This name threw me for a bit before I understood what it was doing. I'm not sure that I necessarily have a better name for it, but wondering if there is one.
NeedsMatch or RequiresMatch? Not really sure those are any better...
There was a problem hiding this comment.
I don't think those are better - they imply that tha match is required, whereas the semantics I'm looking for are "is there a matcher here that will reduce the number of matches?"
| return nil | ||
| } | ||
|
|
||
| func (target TransformTarget) produceTargetType( |
There was a problem hiding this comment.
be consistent with ptr or non-ptr. This one is non-ptr while all the rest are ptr
| } | ||
|
|
||
| func (target *TransformTarget) appliesToType(t astmodel.Type) bool { | ||
| if target == nil { |
There was a problem hiding this comment.
Does this ever actually happen? Do we actually call appliesToType on a nil target? Guarding against this is sorta weird but it'd be weird if this ever happened too?
There was a problem hiding this comment.
It seemed like an easy way to remove boilerplate from calling methods. A nil value can happen if a matcher is never populated (say, if it is not used in the YAML).
| inspect := t | ||
| if target.Optional { | ||
| // Need optional | ||
| opt, ok := astmodel.AsOptionalType(inspect) |
There was a problem hiding this comment.
I don't think this is what target.Optional meant in the old code? Looking at the original TransformTarget, optional was used like this in produceTargetType:
if target.Optional {
result = astmodel.NewOptionalType(result)
}
That's the only place it was used. Maybe you're fixing a bug here where it just was ignored when used on ifType? I think that's what's happening but just wanna confirm.
There was a problem hiding this comment.
That result type was used with a Equals() to see if we had the type that needed to be modified; this approach doesn't work with wildcards. Instead, we check to see if we need optional, and if we do, whether we have it.
| opt, ok := astmodel.AsOptionalType(inspect) | ||
| if !ok { | ||
| // but don't have optional | ||
| return false |
There was a problem hiding this comment.
This could be a bit weird though, because what error is going to result when you set Optional: True, then match a non-optional type. Is it going to say "I couldn't find that property"? That's not actually correct though because it can find it, it just didn't match the optional requirement.
There was a problem hiding this comment.
I'll tweak the error message to say "matched types but all properties were excluded by name or type" (the bold bit is new)
| @@ -36,11 +39,9 @@ type TypeTransformer struct { | |||
|
|
|||
| // IfType only performs the transform if the original type matches (only usable with Property at the moment) | |||
| IfType *TransformTarget `yaml:"ifType,omitempty"` | |||
| ifType astmodel.Type // cache the astmodel type | |||
There was a problem hiding this comment.
Previously this was using produceTargetType for both the IfType and the Target sides of the equation. Then it would produce an astmodel.Type which we wanted to match and use Type.Equals to determine if the match worked (for ifType, and then swap it with the targetType.
While that was sorta "tricky", it did mean that the produceTargetType code was pulling double duty and we could lean on type.Equals. I think I understand that now we can't actually do that because by support wildcards (which type.Equals doesn't understand) we need to do our own matching, hence your AppliesToType. is that right?
What this PR does / why we need it:
Despite the appearance of fragments like this in our configuration
We did not support wildcards within the
ifTypesection; the matching on version*was accidentally working, due to the way our package references filtered out non-alphanumeric characters during creation.In PR #2294 I'm removing this filtering to improve our diagnostics, breaking this match.
This PR fixes the matching to properly work with wildcards. We could do some further work to improve diagnostics, etc, but I figured this was change enough.
I've included caching to improve the performance of the transforms, and to reduce the amount of heap debris, hopefully lowering the amount of time spent doing garbage collection. Crudely measures, this seems to help:
Without caching:
With caching
Special notes for your reviewer:
I've left in tests I wrote during troubleshooting.
How does this PR make you feel:
If applicable: