Re: [Pki-devel] [PATCH] 94 Initial patch with implementation for ProfileClient
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