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
> as it's the only
> supported version of Go.


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

> [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