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