Skip to content
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

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Jul 27, 2017

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

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

@dsnet dsnet Jul 27, 2017

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

@dsnet dsnet Jul 27, 2017

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:

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.

Copy link
Collaborator

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).
@dsnet dsnet requested a review from neild November 2, 2017 18:08
@dsnet dsnet changed the title Trying to serialize transformers with @jba's idea Add implicit filter to Transformers Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants