launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Review: Approve

Quite some code to review!  I really need syntax highlighting.

Just some small notes:

(*) 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.

(*) 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.

(*) 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!

(*) 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)

(*) Typo:

+// newSingleServingServer create a single-serving test http server which will

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