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

[Ade Lee]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel