launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Thanks for the review Jeroen!

> (*) I see you're thinking ahead and returning error codes even where you don't
> need them.  That makes a lot of sense to me.  I forgot it in one or two
> places, and given how brittle Go is to minor API changes, it's relatively hard
> to retrofit an error return code.
> 
> (*) One case where I think I *would* line-break long lines in Go is where a
> map initializer takes up a lot of space.  Not so much because the long line
> bothers me per se, but because the precedence order between "," and ":" isn't
> immediately obvious to the eye.

Good point, done.

> (*) Ow!  Speaking of my eyes...
> 
> +type pLAINTEXTOAuthSigner struct {
> 
> That looks like a mistake with the Caps Lock key.  :)  I would very, very much
> prefer "plaintext" in all lower case.

You're definitely right, this was ugly.  I changed it to _PLAINTEXTOAuthSigner.  I think it's better that to use lower case (because we will have a "SHA1" signer some day).  I've seen in the doc mentioned this option.

> (*) Import "launchpad.net/gocheck", or import . "launchpad.net/gocheck"?
> 
> We talked about this once, and you mentioned that the others do the "."
> import.  But the Go guidelines warn against that sort of import in pretty
> strong language.  In my own tests, I just do a regular import and qualify
> everything as "gocheck.<...>".  This makes it much easier for a new reader to
> find out where things like C come from.
> 
> Maybe I'm wrong, but it'll be easier to change that to the "." style than the
> other way around!

I think I'd like to keep it this way (and so I'd be in favor of you changing your code) for multiple reasons:
- it's the way the Juju people do (I'm not using the authority argument but having a consistent code style would be good).
- even the Go doc says that this kind of import might be useful for tests: (http://golang.org/doc/effective_go.html) "The importer of a package will use the name to refer to its contents (the import . notation is intended mostly for tests and other unusual situations and should be avoided unless necessary), so exported names in the package can use that fact to avoid stutter."
- You're right that "gocheck.C" is better than "*C" but I really don't like the noise that it adds in: "c.Check(result, gocheck.Equals, expectedResult)"

> (*) You use c.Assert() for testing.  I've been using c.Check() because it
> seemed more appropriate to what I was doing.  Any ideas on which to use when?
> 
> My personal impression is: use c.Check() if you're in that final block of
> verification, and one of your items is wrong, but you also want to know if any
> of the following items are wrong too.  Use c.Assert() if the failure is fatal
> to the test and you can't continue from there — e.g:
> 
>         c.Assert(obj, NotNil)
>         c.Check(obj.foo, Equals, 4)
>         c.Check(obj.bar, Equals, szot)

Good point, I used c.Assert everywhere blindly but what you suggest is better.  Done.

> (*) Typo:
> 
> +// newSingleServingServer create a single-serving test http server which will
> 
> That should be "creates".

Done.

> (*) Test setup gets pretty tedious. ISTM that could be shorter with a helper to set up a server and a client.

Not sure what I can do really, this is the typical setup:

	URI := "/some/url"
	expectedResult := "expected:result"
	client, _ := NewAnonymousClient()
	params := url.Values{}
	params.Add("op", "list")
	fullURI := URI + "?op=list"
	server := newSingleServingServer(fullURI, expectedResult, http.StatusOK)
	defer server.Close()

The only thing I could do is create the params in one go using the helper we talked about (which does not exist yet).  But using a help I would simple save one or two lines so I don't think it's worth it.

-- 
https://code.launchpad.net/~rvb/maas/gomaasapi-listnodes/+merge/144654
Your team MAAS Maintainers is subscribed to branch lp:~maas-maintainers/maas/gomaasapi.