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.

Sign up for GitHub

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

Jump to bottom

text/template: make "eq" support all comparable types #33740

Closed
a8m opened this issue Aug 20, 2019 · 7 comments
Closed

text/template: make "eq" support all comparable types #33740

a8m opened this issue Aug 20, 2019 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone
Go1.14

Comments

@a8m
Copy link
Contributor

a8m commented Aug 20, 2019

The current eq function is limited only to "basic types", and it does not support other Go types that are comparable, like pointers or structs.
Also, it's not possible to decorate it, because it's private. Setting "eq": reflect.DeepEqual won't provide the same functionally, because cases like this:

package main

import (
	"log"
	"os"
	"reflect"
	"text/template"
)

func main() {
	tmpl, err := template.New("titleTest").
		Funcs(template.FuncMap{
			"goeq": reflect.DeepEqual,
		}).
		Parse(`{{ eq .A 1 }} != {{ goeq .A 1 }}`)
	if err != nil {
		log.Fatalf("parsing: %s", err)
	}
	err = tmpl.Execute(os.Stdout, struct{ A int64 }{1})
	if err != nil {
		log.Fatalf("execution: %s", err)
	}
	// Output: true != false
}

https://play.golang.org/p/zTg33Lggq0f

My current solution is just copying the current code from text/template/funcs.go, and adding the support by myself.

I would like to hear what others think about this, because it feels like a basic functionality that is missing when working with Go types.

If this proposal will be accepted, I will be more than happy to contribute my changes.
@gopherbot gopherbot added this to the Proposal milestone Aug 20, 2019
@mvdan
Copy link
Member

mvdan commented Aug 21, 2019

I'm not sure I get what you're proposing; the title seems to suggest Go's == for pointers and structs as per the spec, but your example uses reflect.DeepEqual, which is quite a bit more complex than Go's equality operator.

See https://golang.org/pkg/reflect/#DeepEqual and https://golang.org/ref/spec#Comparison_operators.

@a8m
Copy link
Contributor Author

a8m commented Aug 21, 2019

Hey @mvdan, sorry for not being clear.
My proposal is about adding support for all comparable types in the builtin eq function (or at least pointers).

About the example I've added above (with reflect.DeepEqual) -
I tried to demonstrate that as a user of this package, it's not possible for me to decorate (or extend) the existing eq function and add support for other comparable types. The current solution is just to override it.
The quickest way that came to my head was to override it with reflect.DeepEqual, but that was wrong, because the 2 functions don't provide the same functionally as I showed above (eq has this "loose comparison" for numeric types).
So I ended up with copying the eq function from funcs.go and added the support in my code.

Is there any objection about adding this support? or, at least part of it (pointers only)? If not, I would like to contribute my changes.

@ianlancetaylor
Copy link
Member

CC @robpike

@griesemer griesemer mentioned this issue Sep 3, 2019
Open
@robpike
Copy link
Contributor

robpike commented Sep 4, 2019 •
edited
Loading

It's too bad that comparison of interface values panic if the types aren't comparable, and there is no way to avoid that bar catching the panic. If we were willing to catch the panic, which would be expensive but probably very rare in practice, we could just do return v1 == v2 in the default clause of the switch k1 case in the eq function.

That might be the right solution. It wouldn't require baking in lots of information about what types are comparable.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193837 mentions this issue: text/template: support all comparable types in eq

@rsc
Copy link
Contributor

rsc commented Sep 12, 2019

Given @robpike's comment above, this sounds like a likely accept.
Leaving open for a week for final comments before accepting.

@rsc
Copy link
Contributor

rsc commented Sep 18, 2019

No comments, so proposal accepted.

@rsc rsc changed the title proposal: text/template: make "eq" support all comparable types text/template: make "eq" support all comparable types Sep 18, 2019
@rsc rsc modified the milestones: Proposal, Go1.14 Sep 18, 2019
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. help wanted and removed Proposal-FinalCommentPeriod labels Sep 18, 2019
@ALTree ALTree mentioned this issue May 16, 2020
Closed
@golang golang locked and limited conversation to collaborators Sep 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
6 participants
@rsc @ianlancetaylor @mvdan @robpike @a8m @gopherbot