Code review comment for lp:~julian-edwards/gwacl/updatehostedservice

On 04/07/13 16:55, Raphaël Badin wrote:
> Review: Approve
>
> Thanks, nice change.

Thank you.

>
> [0]
>
> +func (api *ManagementAPI) UpdateHostedService(serviceName string, params *UpdateHostedService) error {
> 11 + var err error
> 12 + URI := "services/hostedservices/" + serviceName
>
> You forgot to call checkPathComponents(serviceName)

Ok thanks.

>
> [1]
>
> 11 + var err error
>
> No need for this declaration I think.

Well - it kinda is. When you do multiple assignments to err, if it's
the second return value in a := statement it doesn't get set
consistently in my experience - at least it's fucking confusing me
anyway, assignment semantics in Go are batshit. Adding the var ensures
it's never masking the result with subsequent assignments.

>
> [2]
>
> This can be done later but it would be great to use this method in example/management/run.go.

Indeed, but the branch was large enough.

>
> [3]
>
> [2]
>
> 152 +func (suite *xmlSuite) TestUpdateHostedService(c *C) {
> 153 + label := MakeRandomString(10)
> 154 + expected := makeUpdateHostedService(label)
> 155 + input := UpdateHostedService{
> 156 + XMLNS: XMLNS,
> 157 + Label: label,
> 158 + Description: "description",
>
>
> This is a detail but it's a bit weird that "description" references a string that is buried deep in the XML code above, maybe the description (and the other values of the XML string) should be an argument of makeUpdateHostedService().
>

True, and I was just harping on about that in IRC as well :) I fixed
it, thanks for point it out.

J