Code review comment for lp:~allenap/gwacl/remove-role-endpoints

Gavin Panella (allenap) wrote :

> [0]
...
> I had not grokked any of that (but could be Lyme brain) from the docs.  It's
> madness.

Ha :) I've tried to improve the comment here. However, as you've
probably seen, the approach is still being discussed, so don't pay too
much attention to this yet.

>
> [1]
>
> +    panic("wtf") // Safe to remove in Go 1.1
>
> Should be OK to remove now, we are all supposed to be using Go 1.1 (add
> https://launchpad.net/~james-page/+archive/golang-backports) as it's the only
> supported version of Go.

Done.

We can also upgrade the code underlying fork/http and fork/tls now;
it's based on 1.0.2 right now, for compatibility. Rapheël has already
tried this and it works fine, iirc, we just need to get round to doing
it.

>
> [2]
>
> +// Returns true to keep the endpoint defined in the role's configuration,
> +// false to remove it. It is also welcome to mutate endpoints; they are
> passed
> +// by reference.
>
> By pointer, strictly speaking.

Is this a distinction that only makes a difference to a former C++
programmer, or is there a tangible difference in meaning?

>
> Very nice readable tests for the new comparison func, BTW!

Ta :) Very nice review :)

« Back to merge proposal