launchpad-reviewers team mailing list archive

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

Review: Approve

Thank you for applying this last-minute bit of polish.  It'll save us time.

Review notes:

 * I think now you'll have to capitalize "json": newJSONMAASObject()!

 * [In maasobject.go] newjsonMAASObject() is structured in a way that's probably harder to follow than necessary.  The Go style is for error conditions to break out of the function, so that the function body can continue with the normal execution path — without accumulating "if" nesting and indentation.

Current code:

«
func newjsonMAASObject(jmap jsonMap, client Client) jsonMAASObject {
	if uriObj, ok := jmap[resource_uri]; ok {
		uriString, err := uriObj.GetString()
		if err != nil {
			panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a string.")
		}
		uri, err := url.Parse(uriString)
		if err != nil {
			panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a valid URL.")
		}
		return jsonMAASObject{jmap, client, uri}
	}
	panic("Cannot create jsonMAASObject object, no 'resource_uri' key present in the given jsonMap.")
}
»

What I would suggest:

«
func newjsonMAASObject(jmap jsonMap, client Client) jsonMAASObject {
	uriObj, ok := jmap[resource_uri]
	if !ok {
		panic("Cannot create jsonMAASObject object, no 'resource_uri' key present in the given jsonMap.")
	}
	uriString, err := uriObj.GetString()
	if err != nil {
		panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a string.")
	}
	uri, err := url.Parse(uriString)
	if err != nil {
		panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a valid URL.")
	}
	return jsonMAASObject{jmap, client, uri}
	}
}
»

Nice and flat.  Also helps with those long lines.  :)

(I did it without the "if x, ok := foo(); ok" idiom because I don't know if that is also considered the right thing to do for the not-ok case.  So read whatever is more Go.)

 * [In maasobject_test.go] Since capitalization of identifiers is effectively part of the syntax in Go, not really part of the identifier as such, I'd also re-case the "new" in "TestnewjsonMAASObjectPanicsIfNoResourceURI."

Well done on the panic tests!


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