[Merge] lp:~mandel/udm/add-tests into lp:udm

Sergio Schvezov sergio.schvezov at canonical.com
Tue May 13 18:22:50 UTC 2014


Review: Needs Fixing

41	+type watch interface {

for quick reading can you call this watchInterface or something of the sort? Same for proxy perhaps

43	+ Chanel() chan *dbus.Message

Channel?


+ return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)

errors should always be lower case to be able to concatenate better in logs et.al. DBus is fine since it's a proper name, but 'Error' should be 'error'


--
func (down *FileDownload) Throttle() (throttle uint64, err error) {
	reply, err := down.proxy.Call(DOWNLOAD_INTERFACE, "throttle")
	if err != nil {
		return 0, err
	}

	if reply.Type == dbus.TypeError {
		return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)
	}

	if err = readArgs(reply, &throttle); err != nil {
		return 0, err
	}
	return throttle, nil
}

How about

func (...) Blah() (...) {
    return down.getUint64Value("throttle")
}

func (...) getUint64Value(valueName string) (uint64, error) {
	reply, err := down.proxy.Call(DOWNLOAD_INTERFACE, valueName)
	if err != nil {
		return 0, err
	}

	if reply.Type == dbus.TypeError {
		return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)
	}

        var value uint64
	if err = readArgs(reply, &value); err != nil {
		return 0, err
	}
	return value, nil
}

rinse and repeat for progress, totalSize, et.al; similar logic for others

595	+ . "gopkg.in/check.v1"

As mentioned in the previous Needs Fixing, you need to use launchpad.net/gocheck


Finally, can you run goimports against the code? It formats internal and external imports nicely.
-- 
https://code.launchpad.net/~mandel/udm/add-tests/+merge/219200
Your team Ubuntu Phablet Team is subscribed to branch lp:udm.



More information about the Ubuntu-reviews mailing list