Code review comment for lp:~julian-edwards/launchpad/api-expose-builders

Revision history for this message
Leonard Richardson (leonardr) wrote :

This looks good. Just some nitpicky stuff:

On the web service, getByName is a named operation (or "operation"), not a method. Calling it a method makes it look like a local method call.

'Our "bob" builder was retrieved using the no-priv user, and he does not..' The "he" is ambiguous. s/he/no-priv/.

Do you want to iterate over a *sorted* list of attribute names? bob.lp_attributes is ultimately dependent on the order of attributes in the IBuilder definition. Something to think about.

You might want to distinguish between cprov's launchpad and no-priv's in the variable names. 'launchpad' and 'cprov_launchpad' or 'privileged_launchpad'.

« Back to merge proposal