Code review comment for lp:~rvb/gwacl/list-endpoints

On Monday 05 Aug 2013 21:52:27 Gavin Panella wrote:
> Review: Approve
>
> [1]
>
> + endpointsP := vmRole.ConfigurationSets[i].InputEndpoints
>
> I assume the P suffix denotes a pointer. I'm not sure I like that, but
> don't change it; I don't have a reason to object other than aesthetics
> really.

In the olden (some say golden) days of C and C++, we used to have pMyVariable. If
thingP is idiomatic, I wonder what motivated Pike and Co. to do it differently? I am
guessing the same reason they made a bunch of other things unnecessarily
different.

>
>
> [2]
>
> +func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
> ...
> + c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
>
> I wonder if we should revisit these behaviours everywhere in GWACL and
> return nil instead of an error. That's easier to use in a conditional,
> and a common idiom for absense of something. Errors are a pain to make
> decisions about, except nil or not. What do you think? (Utterly out of
> scope for this branch; I'm just seeking your opinion.)

I think there's an argument both ways here. Returning a nil is useful in the scenario
you describe, for sure. Conversely, I am loathe to remove information from the
caller. Gwacl needs to be a bit lower-layer than that, or its use starts to get biased
to a certain kind of application.

I do like the idea of the "wrapper" functions in the non "_base" files. Perhaps we
just need more of those instead?