Code review comment for lp:~jtv/gwacl/list-images

On Tuesday 02 Apr 2013 03:25:27 you wrote:
> Well the point is of course, as you saw right after writing this, that they
> had to be repeated exactly.
> > 80 + c.Check(osimage.LogicalSizeInGB, Equals,
> > float32(logicalSize))
> > 81 + c.Check(osimage.Label, Equals, label)
> > 82 + c.Check(osimage.MediaLink, Equals, mediaLink)
> > 83 + c.Check(osimage.Name, Equals, name)
> > 84 + c.Check(osimage.OS, Equals, "Linux")
> > 85 + c.Check(osimage.EULA, Equals, eula)
> > 86 + c.Check(osimage.Description, Equals, description)
> >
> > You can do this in a single check if you make a single "expected" struct
> > and do a DeepEquals, as is done for some of the storage tests. The code
> > reads better that way and you also get a single failure listing all
> > differences at once.
>
> Done.

What you've done is loads better now, thanks :)

>
> > 103 + c.Assert(len(listing), Equals, 3)
> > 104 + c.Check(listing[0].Name, Equals, "image2")
> > 105 + c.Check(listing[1].Name, Equals, "image1")
> > 106 + c.Check(listing[2].Name, Equals, "image3")
> >
> > Same here.
>
> Can't DeepEquals the objects here, or the error output will be completely
> impossible to figure out. After all what's in the objects really doesn't
> matter — only the ordering does. So instead I created one array with the
> desired names and another with the received names, and compared those.

Oh, arse :/

>
> Extracting an array of Name attributes from the array of objects does make
> the code long and cumbersome, of course. But such is the price of Go:
> don't struggle, you'll just sink faster. The more generic and data-driven
> I make this test, the worse it gets.

:(

Our best practices are much harder to implement in Go than we are used to with
Python.

> > 122 +// rigPreparedResponseDispatcher sets up a request dispatcher
> > that
> > returns,
> > 123 +// for each consecutive request, the next of a series of prepared
> > responses.
> >
> > This reminds me of Mocker. Out of interest, do you prefer its
> > record/replay mocking style over the simpler Mock?
>
> I don't think I've ever used this function myself, so I'm probably not the
> best person to ask. I just saw that its documentation was broken and fixed
> it.

Ok :)

> > +type OSImage struct {
> > 154 + AffinityGroup string `xml:",omitempty"`
> > 155 + Category string
> > 156 + Label string
> > [snip]
> >
> > You said you were leaving out the xml tags, but please don't do that.
> > Gavin and I talked about this in Brisbane too, but we decided that we
> > need a strong separation between external API names and internal code
> > struct names - if the Azure API changes then it would either mean code
> > changes everywhere or adding the tag anyway. Adding the tags also
> > reinforces to the code reader that its an XML-filled property.
>
> Okay, I put them in. A minor disadvantage is that this obscures the
> differences in capitalization between gwacl and Azure.

It does?

Thanks for the changes, go ahead and land!