On Fri, 2014-05-23 at 15:53 +1000, Fraser Tweedale wrote:

On Thu, May 22, 2014 at 02:38:48PM -0400, Abhishek Koneru wrote:

...

Please review the attached patch with the implementation of ProfileClient.

The following methods are implemented in this patch - list_profiles - Returns a list of profiles. get_profile - Retrieves information of a profile. enable_profile - Enables a profile disable_profile - disables a profile.

Also includes some cosmetic changes account.py and client.py.

Thanks, Abhishek

Hi Abhishek,

I'm happy with the overall feel of the API. Some comments follow.

1) Copy pasta error in ProfileData.from_json; ``policy_sets`` used instead of ``inputs``/``outputs`` when processing json_value['Input'] or json_value['Output'] lists.

2) Could you please make ProfileDataInfoCollection an iterable? It doesn't make sense to be to have to access the ``profile_data_list`` attribute just to iterate the ProfileDataInfo objects.

3) Could you please add a ``repr`` method to ProfileData that includes at least the profile_id? Maybe summary information about whether it is visible/active as well, but I think just the ID should be fine. Same goes for other classes that show up in lists, please.

4) I'm a little concerned about having the properties set/get top-level attributes, e.g. ``enabled_by`` sets/gets ``enabledBy``. Following this pattern where you wish to use the same name as in the JSON would result in infinite loop; indeed you do treat some keys different to avoid this, e.g. ``description``. This inconsistency makes me wonder if there's a better pattern.

My suggestion would be to stash the JSON dict in a top-level attribute (the constructor would have to initialise it to an empty dict before other attributes are assigned) and then have the properties set/get items in that dict.

You could further cut down boilerplate and duplication by definite a descriptor for this purpose, e.g.:

class JSONProperty(object): def __init__(self, attr): self.attr

def __get__(self, instance, owner) return obj.json_value[self.attr]

def __set__(self, instance, value) obj.json_value[self.attr] = value

Then, most of the assignments in ``from_json`` go away , and the corresponding property declarations follow the new pattern:

enabled_by = JSONProperty('enabledBy')

You would still need to treat Input, Output and PolicySets differently, but you could also abstract over this pattern with yet another class, i.e. a "JSON list property".

Anyhow, 4) isn't a show-stopper, just a (lengthy) nit-pick.

I'd like to consider this approach or another way to solve this as well. Right now, it is a little confusing. Basically the rule is - if the json name is different from the python attribute name, and that parameter may be sent back to the server (rather than just being read on the client side, then it needs a property/setter) It would be nice to have a more uniform mechanism of mapping the attributes - and to be able to avoid a lot of the boilerplate code in to/from_json. Lets try address this now, because its a pattern that is being set in cert.py, key.py and profile.py.

Cheers,

Fraser

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel