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. Cheers, Fraser