-
Notifications
You must be signed in to change notification settings - Fork 209
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
Add implicit filter to Transformers #29
Conversation
cmp/options.go
Outdated
// are the same), an implicit filter is added such that a transformer is | ||
// applicable only if that exact transformer is not already in the | ||
// tail of the Path since the last non-Transform step. | ||
// While this is sufficient for most usages of Transformers, this does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caveat is unfortunate, and shouldn't be necessary. Also, describing the rule in terms of paths is technically precise but hard to understand intuitively. I would change the algorithm to better match mine (namely, each transformer can only be applied once per value per cmp.Equal call) and describe it in something like those terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you concerned about how it is documented here? Or are you saying that the implementation is not equivalent to what you had in mind?
If the former, we can adjust the prose written.
If the later, what makes this current implementation different?
I'm just trying to go for a minimal implementation so that we can test out ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both.
I haven't looked at your actual code, but my thinking is that once you apply a transformer to a value in a given invocation of cmp.Equal, you shouldn't apply it again, whether or not it changes the path.
Basically, I'm thinking in terms of invocations of cmp.Equal, not path changes. Every invocation of cmp.Equal changes the path, but not every path change is the result of a call on cmp.Equal. I didn't realize until this discussion that you put things like type assertions and transforms in the path, and I don't totally get why you do, but in any case I'd ignore all the path steps that don't correlate with a call on Equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to have Transforms and TypeAssertions be part of the Path since they get printed. Otherwise you end up with weird situations when you know that the struct field type was an int, but the Path says its a string because there was a hidden transformation in between.
For that reason, there is a one-to-one mapping of recursive calls to cmp.Equal and path steps:
Indirect
for pointersTypeAssertion
for interfacesSliceIndex
for slicesMapIndex
for mapsStructField
for structsTransform
for transformations
The steps Indirect
, TypeAssertion
, SliceIndex
, MapIndex
, and StructField
correspond directly with recursive calls dictated by the structure of the value themselves. Only Transform
is special.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see your point. I withdraw most of my criticism, though I still wish the doc would explain this more clearly.
Declaring a transformer "func(T) T" where T is a concrete type is a common transformation. However, this is currently problematic as the transformation now infinitely applies to itself recursively. In order to allow this form of transformation, add a simple (but subtle) filter to Transformers where a Transformer can only apply if that specific Transformer does not already exist within the tail of the current Path since the last non-Transform step. This rule does not prevent more advance usages of Transformers where the user *does* want the Transformer to apply recursively to previously transformed output, since those situations are almost always followed by some path step that is *not* a transformation (e.g., pointer indirect, slice indexing, etc).
7523d91
to
c2186a4
Compare
Declaring a transformer "func(T) T" where T is a concrete type
is a common transformation. However, this is currently problematic
as the transformation now infinitely applies to itself recursively.
In order to allow this form of transformation, add a simple
(but subtle) filter to Transformers where a Transformer can only
apply if that specific Transformer does not already exist within
the tail of the current Path since the last non-Transform step.
This rule does not prevent more advance usages of Transformers
where the user does want the Transformer to apply recursively
to previously transformed output, since those situations are
almost always followed by some path step that is not a
transformation (e.g., pointer indirect, slice indexing, etc).
Fixes #17
Fixes #51