Test needs to compare contents of struct

[alecthegeek]

Previously we were comparing the address of the two test structures, which always failed.

[alecthegeek]

Please be careful with this change -- My go skills are very limited. However this is the only
way I can make the tests pass when the two clocks have the same underlying values

[alecthegeek]

is this change version specific? I am running Go 1.4 locally

[alecthegeek]

This appears to fail because I implemented clock as a struct instead of an int

[kytrinyx]

You're absolutely correct that we should compare values of type clock rather than pointers to values of type clock.

The language specification states:

Pointer values are comparable. Two pointer values are equal if they point to the same variable or if both have value nil.
[...]
Struct values are comparable if all their fields are comparable. Two struct values are equal if their corresponding non-blank fields are equal.

The existing example passes the earlier version of the tests, because New does not return a pointer to a clock, but a value of type clock.

In idiomatic Go New would return a pointer to a clock, so the example solution (in clock/example.go) should really changed to reflect this. Additionally, the method receivers for the Add and String methods should also be pointers to Clock.

That would get the tests passing again.

[soniakeys]

Pointers don't need to be used as much in Go as in some other languages. Common advice for example is to prefer value receivers over pointer receivers where there is no compelling reason to use pointers. An example closer to this exercise is the way the time.Time type is used as a value rather than a pointer to a value. I think a lesson for this exercise in Go is appreciating the use of values in Go. This issue on this exercise has come up a couple of times though. I think it might be appropriate to add comments to the top of the test program explaining that a working solution will work with values directly rather than indirectly and give some links for further reading -- the time.Time type for sure, and something comparing value receivers to pointer receivers. I'll look this up and find some links...

[kytrinyx]

Common advice for example is to prefer value receivers over pointer receivers where there is no compelling reason to use pointers.

Interesting. In my team at work we tend to prefer pointers, and I wasn't aware of the advice for values.

I do think that I've seen somewhere that the expectation for New is that it returns a pointer to a value, not the value itself. I'll see if I can find the reference.

[soniakeys]

https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

then see how time.Time methods have value receivers

http://golang.org/pkg/time/#Time

We maybe could push this to nitpicking though by using reflect.DeepEqual as we have done with some other exercises.

package main

import (
    "fmt"
    "reflect"
)

type clock int

func main() {
    a := clock(3)
    b := clock(3)
    fmt.Println(a == b)
    pa := &a
    pb := &b
    fmt.Println(pa == pb)
    fmt.Println(reflect.DeepEqual(pa, pb))
}

outputs

true
false
true

[soniakeys]

Ah, well, but earlier discussion on using DeepEqual: #108

[soniakeys]

We never persued dchapes's last suggestion to add something to the failure message. I don't think we've done anything like this yet (not in the Go exercises anyway) but when == fails, we could then test for DeepEqual, and it if passes, then give a failure message suggesting to read comments in the test program, which would clarify that values must compare == and give links such as the two I gave above. It would be adding a little hint or coaching toward a valid solution.

[kytrinyx]

I completely forgot about that suggestion. I really like how that would make the test suite even more useful in pointing people to interesting and useful parts of the language, even before it gets to the nitpicking.

[soniakeys]

Alec, Thank you for raising this issue. We took a different direction with PR #125 but you prompted us to improve the exercise.