Hi Geert, On 11/26/2011 07:49 PM, Geert Jansen wrote: > Hi Michael, > > i just wrote a small program with the new Python SDK. I found it very easy to use, so great work. I do have a few points of feedback: > > * Probably we want all Exceptions derived from a single base class. That way, the user can intercept any ovirtsdk with a single try/except clause. Currently that's not > possible, as all your exceptions derive directly from Exception. sure, in my TODO list.

> * I think the .get() method should be id-based not name based. Using "id" as a primary key has a number of advantages over name: > > - It is immutable, which makes e.g. renaming a lot easier. > - It allows you to retrieve an object without a search query or a filter. > - It matches how the API works as well. "id" is supported, you can do api.collection.get(id=xxx), only as you mentioned "id" is not primary, but secondary key (i thought that searching by name will be easier), but i can change it in a second if this is an issue. > Also currently i think it's not possible to retrieve a resource by its ID, unless you use the id= keyword argument to list() which retrieves the whole collection and then > filters out the matching id. nope, it's supported - i even added it to examples.py [1] -> vm5 = api.vms.get(id='02f0f4a4-9738-4731-83c4-293f3f734782') [1] will add it to get method .get(id, name) as you suggested. > * You may want to introduce some non-entity related operations, like ping(), and connect(). These allow you to better control when you connect, and allow an program to > test if the connection details work. That helps client except the right exceptions at the right time, and display more relevant error messages. - no need for explicit connect() as its done implicitly on proxy construction - no need for ping() as any attempt to use disconnected (and not recoverable) proxy, will cause ConnectionError, so user should only /Trap/ this exception for disaster_recovery > * There's two Python objects involved for each API object. For a VM for example, you have params.VM and brokers.VM. This distinction is made visible to the user, for example: > > template = api.templates.get('Blank') > > vm = params.VM() > vm.name = 'foo' > vm.template = template # ERROR yep, you right doing this is in my TODO list as well (had to switch to CLI, so i do have few leftovers[1] - this is one of them) [1] they all have simple workarounds (like this one as described below) > The last line need to be: > > vm.template = params.Template(id=template.id) for now (as workaround you can pass 'superclass', e.g vm.template = template.superclass), but i suggest you just wait for another day till I'll support this. > In my version of the Python API i used mix-in classes to add the OO behavior to the generated classes. It was easy, because PyXB had support for that. This gave me the > single class behavior. I am not sure if GenerateDS generated code allows that. I'm pretty sure it could be done though, but you may need to do some Python magic. no need, i do this with regular inheritance pattern > * Nitpick: should this just be called "ovirt" instead of "ovirtsdk"? initially it was, but since both cli& sdk would operate in same namespace, i wanted to avoid future collisions and renamed them to ovirtsdk/ovirtcli. > Overall great work though! 10nx a lot mate. > Regards, > Geert > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel

Hi Geert, On 11/26/2011 07:49 PM, Geert Jansen wrote: > Hi Michael, > > i just wrote a small program with the new Python SDK. I found it very easy to use, so great work. I do have a few points of feedback: > > * Probably we want all Exceptions derived from a single base class. That way, the user can intercept any ovirtsdk with a single try/except clause. Currently that's not > possible, as all your exceptions derive directly from Exception. sure, in my TODO list. > > * I think the .get() method should be id-based not name based. Using "id" as a primary key has a number of advantages over name: > > - It is immutable, which makes e.g. renaming a lot easier. > - It allows you to retrieve an object without a search query or a filter. > - It matches how the API works as well. "id" is supported, you can do api.collection.get(id=xxx), only as you mentioned "id" is not primary, but secondary key (i thought that searching by name will be easier), but i can change it in a second if this is an issue. > > Also currently i think it's not possible to retrieve a resource by its ID, unless you use the id= keyword argument to list() which retrieves the whole collection and then > filters out the matching id. nope, it's supported - i even added it to examples.py [1] -> vm5 = api.vms.get(id='02f0f4a4-9738-4731-83c4-293f3f734782') [1] will add it to get method .get(id, name) as you suggested. > > * You may want to introduce some non-entity related operations, like ping(), and connect(). These allow you to better control when you connect, and allow an program to > test if the connection details work. That helps client except the right exceptions at the right time, and display more relevant error messages. - no need for explicit connect() as its done implicitly on proxy construction - no need for ping() as any attempt to use disconnected (and not recoverable) proxy, will cause ConnectionError, so user should only /Trap/ this exception for disaster_recovery > > * There's two Python objects involved for each API object. For a VM for example, you have params.VM and brokers.VM. This distinction is made visible to the user, for example: > > template = api.templates.get('Blank') > > vm = params.VM() > vm.name = 'foo' > vm.template = template # ERROR yep, you right doing this is in my TODO list as well (had to switch to CLI, so i do have few leftovers[1] - this is one of them) [1] they all have simple workarounds (like this one as described below) > > The last line need to be: > > vm.template = params.Template(id=template.id) for now (as workaround you can pass 'superclass', e.g vm.template = template.superclass), but i suggest you just wait for another day till I'll support this. > > In my version of the Python API i used mix-in classes to add the OO behavior to the generated classes. It was easy, because PyXB had support for that. This gave me the > single class behavior. I am not sure if GenerateDS generated code allows that. I'm pretty sure it could be done though, but you may need to do some Python magic. no need, i do this with regular inheritance pattern > > * Nitpick: should this just be called "ovirt" instead of "ovirtsdk"? initially it was, but since both cli & sdk would operate in same namespace, i wanted to avoid future collisions and renamed them to ovirtsdk/ovirtcli. > > Overall great work though! 10nx a lot mate. > > Regards, > Geert > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel