[Pki-devel] [PATCH] 94 Initial patch with implementation for ProfileClient

[Fraser Tweedale]

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