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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
