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

Make it easier to do same-type transforms #51

Closed
jba opened this issue Nov 1, 2017 · 5 comments
Closed

Make it easier to do same-type transforms #51

jba opened this issue Nov 1, 2017 · 5 comments

Comments

@jba
Copy link
Collaborator

jba commented Nov 1, 2017

I wanted to treat time.Times as rounded to the nearest microsecond. I first tried a transform, but it got into an infinite loop (as advertised). I had to add a FilterPath in front of it. This turned a one-line option into a ~10-line option.

So how about adding something like this to cmpopts:

// TransformOnce applies f, which should be of type func(T) T, to values of type T.                                                                                                            
// Unlike Transform, f is applied only once to each original value.                                                                                                                            
func TransformOnce(name string, f interface{}) cmp.Option {
    return cmp.FilterPath(func(p cmp.Path) bool {
        if len(p) == 0 {
            return true
        }
        xform, ok := p[len(p)-1].(cmp.Transform)
        if !ok {
            return true
        }
        return xform.Name() != name
    }, cmp.Transformer(name, f))
}

I know we discussed this earlier, but I still don't really understand why this isn't the default behavior of transformers.

/cc @bcmills

@jba jba changed the title Make it easier to do same-type transforms. Make it easier to do same-type transforms Nov 1, 2017
@dsnet
Copy link
Collaborator

dsnet commented Nov 1, 2017

My comment on #17 (comment) proposed the following rule:

  • Every Transformer has an implicit filter on it where it is only applicable if that specific Transformer does not already exist within the tail of the current Path since the last non-Transform step.

That rule would be mostly backwards compatible (at least it affects no usages inside Google) and I can't imagine why anyone would want the behavior that it would prevent).

I know you disagree slightly and believe it should be "since the last non-(Transform or TypeAssertion or Indirect) step", but I like the simplicity of my proposed rule. In practice, I don't think the difference is going to matter.

If we accept that proposed rule change (implementation in #29 has been pending for a while), then you won't need TransformOnce.

@dsnet dsnet added the duplicate label Nov 1, 2017
@neild
Copy link
Collaborator

neild commented Nov 1, 2017

The proposed rule seems fine to me, if somewhat distressingly subtle.

Wouldn't a simpler approach to rounding time.Times be to use a Comparer instead of a Transformer?

@jba
Copy link
Collaborator Author

jba commented Nov 1, 2017

@neild I have the comparer, but I wouldn't call it simpler than using a transform (assuming transforms behave in the new way @dsnet is describing). The transform only has to think about one value, but a comparer has to think about two. In this case it's a one-line difference because time.Round already exists, but it can be more in general.

@dsnet
Copy link
Collaborator

dsnet commented Nov 2, 2017

\cc @bcmills are you okay with making the policy change from #51 (comment)?

@bcmills
Copy link

bcmills commented Nov 2, 2017

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants