> The base URL being passed down to MAASObject is surprising to me. I would > have expected the base URL to be encapsulated in the Client so that the > client's Get()/Post()/Put()/Delete() are inherently relative to the base URL. > (Or if we ever get absolute resource URIs, it'd be up to the Client to > reconcile those with its base URI). That is also what we did in the python > API wrapper.
Thanks for the review. Let's talk about this and see what we can do about it. I've fixed a bunch of stuff and left some others so that we can fix them in follow-ups branches: > 1. [In client.go] Could you document modification()? There may even be some > better name for it, which better fits the notion that Delete() isn't a > modification(). I've renamed it "nonIdempotentRequest" and added a comment. > > 4. [In live_example.go] I think you're making a sensible choice by just > panicking if something goes wrong. It helps keep the code brief. But maybe > this deserves a top comment, so that somebody else doesn't think it's by > accident. If they helpfully add error checking that's fine, but it's not so > fine if they base their own programs on this code, or file bug reports about > the missing error checks. Done. > 5. [In live_example.go] Is there a clear warning that this example messes with > the data on the server? If deletes the node it creates once it's done, but > that's not much help if for whatever reason the program does not complete. Done. > 6. [In maasobject.go] The name SubObject() confuses me. It looks like two > words, in which case I would really expect the first word to be a verb. How > about GetSubObject()? Good point, done. > > 9. [In maasobject_test.go] SubObject() is untested. Added a test. > 10. [In server.go] NewServer() is undocumented, and its purpose is not very > clear — it looks like a constructor for a nonexistent Server class. I would > describe the function as "presenting an instance of the MAAS API as a > MAASObject," but that doesn't use the word "server" at all. In fact I > wouldn't use that word here, because the MAAS terminology no longer includes > it. "Server" could refer to a node, a cluster controller, a region > controller, or some other server system with roughly equal validity. Good point, done (NewServer => NewMAAS). > 11. [In testing.go] Not something you did, in fact you're already improving > this line, but if you have time: "404 page not found" is a horrible error > message. It indicates that the 404 page was not found, which is probably not > what was meant! Some punctuation between the error number and the error text > would be enough to resolve this. (Actuall http.StatusNotFound is just as bad, > but there's little we can do about that.) Not yet done: > 2. [In client.go] Why accept an empty "operation" string? Given that this is > the MAAS API, that looks more like an error to me. > > 3. [In client_test.go] TestClientDeleteSendsRequest does not verify that the > right request is made, only that Delete() returns no error when you expect no > error. You can't test the request as easily as you can with the other > methods, but you can verify that Delete() does return an error when the > request is to the wrong URL. In Python we would test a good proportion of > such error conditions, but in Go it looks like the overhead of testing is so > high that we have to pay attention to what's worth testing and what isn't. :( > 7. [In maasobject.go] Does Post() sometimes return something that is not a > MAASObject? I gave it a quick look when I wrote the object-handling code, but > what I found suggested that a non-method POST conventionally returns the > object you just posted. (A POST to a method call of course may return lists > and other JSON). > > 8. [In maasobject.go] When panicking in response to an error, it'd be nice to > include the error message. -- https://code.launchpad.net/~rvb/maas/gomaasapi-junction/+merge/145001 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
