Code review comment for lp:~jtv/gwacl/service-endpoints

On Tuesday 06 Aug 2013 08:02:26 you wrote:
> I disagree. When a new region appears with "China" in its name, there's a

[snip]

Ok I just thought I'd bring it up. I think we'll end up getting bitten one way or
another but hey ... SEP.

> > 60 +// StorageAPI returns the base URL for the endpoint's storage
> > API.
> > 61 +func (endpoint APIEndpoint) StorageAPI() string {
> >
> > Why not a pointer receiver? (same for ManagementAPI)
>
> Because it's the language's way of saying that you're not going to get your
> object modified by calling this. I don't care much about it either way.

Ok - it's just that we know that non-pointer receivers don't meet the interface etc
etc. Most of the rest of the code uses pointer receivers.

> > Also, the storage URL returned here is still not complete, it would need
> > the account prefixed.
> > It might be worth encapsulating that functionality in this object so that
> > all the URL
> > manipulation is done in the same place. I'd have StorageAPI take an
> > account name parameter.
>
> We do need that, but I felt that was in a different layer of responsibility.
> We already had a method for it, which I kept in place (although I made it
> re-use prefixHost so it's very simple).

I genuinely don't think it's a different layer of responsibility.

My reasoning is that each storage account is a different API endpoint. This code
says it returns an API endpoint, but it doesn't, it's not usable yet.

> This test verifies a specific relationship between the function's responses
> to different inputs. Not the actual responses themselves; those are
> covered by tests at the appropriate level and they're not relevant here.
> This is just a higher level of abstraction and I don't think that's a bad
> thing.
>
> Making the change you ask for would replace an explicit statement of the
> relationship with one that is implicit and easily lost in the unnecessary
> detail. But it also increases fragility. For example, we might at some
> point remove the trailing slashes after the URLs' hostnames. Completely
> arbitrary change, but simplestreams might force us to make it someday.
> That is completely outside the scope of this test: the test shouldn't care.
> But the way you're asking me to write it, it'd break this test for no
> reason. An engineer with a whole bunch of these failures on their hands
> would have to fix up the URLs in the tests on auto-pilot.
>
> On the other hand, imagine that the *relationship* that this test verifies
> breaks during development. The test would fail, because that's what it's
> for. But if I wrote it your way, the required change would be
> indistinguishable from the change required for the inconsequential change
> from the first example. What is the engineer's reasonable response? Fix
> up the URLs in the tests, on auto-pilot. At that point the test becomes
> meaningless -- because the relationship that's being tested was left
> implicit in the data instead of explicit, and because of the unnecessary
> fragility.

Ok we'll have to agree to disagree.

> > I don't like the "try to set this" stuff at all here. Let's just panic if
> > it's not set.
>
> Neither do I. See the merge proposal.

I was agreeing with your merge proposal :)

>
> > Out of interest, I wonder if Go works like C++ here and lets you supply a
> > value that works with
> > the type's constructor as well as supplying the type itself.
>
> I don't know what you mean.

In C++ I can pass a type to a parameter that can be used to construct the
parameter object on the fly.

So I was wondering if you could specify a string where APIEndpoint is the type. I
expect Go is not clever enough to deal with that though.

> > And as I mentioned before, let's encapsulate this in APIEndpoint.
>
> Hmm... I tried doing that just now, but it made things appreciably worse,
> because of the way it breaks down separation of responsibilities. Instead,
> I documented the need to prefix the account name.

I'll take a look myself tomorrow. I had something in my head that felt like it would
simplify a lot of stuff.

Cheers.