[Pki-devel] [PATCH] 95 Fixes for comments for patches 92-2, 93, 94(CertClient and ProfileClient python implementations)

[Fraser Tweedale]

On Thu, May 29, 2014 at 04:03:14PM -0400, Abhishek Koneru wrote:
> Please review the patch which addresses the review comments given by Ade
> and Fraser for patches 92-2, 93, 94.
> 
> 92-2 has been checked in already. 94 has been rebased to fix conflicts
> with Ade's checkins. I removed the changes to account.py and client.py
> form 94 to fix the conflicts. 
> 
> So update the master and apply patches 93, 94, 95.
> 

Ran into some more issues with patch 93:

1) there is an unused class ``ProfilePolicySet``.  Is the purpose of
this class the same as ``PolicySet``?  If so, they should be merged.

2) as currently implemented, ``PolicySet.from_json`` has no explicit
return, therefore returns None

3) ``PolicySet.from_json`` likewise lacks a return statement.

Cheers,

Fraser

> Following are the review comments addressed in this patch -
> 
> ** No. 4 in the list is not included in this patch. It will be done in a
> separate patch.
> 
> 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.
> 
> *** Will fix this in a separate patch.
> 
> 5. Here you could iterate the (key, value) pairs to save keystrokes and
> a bit of CPU doing ``cert_search_params[param]`` in all those places
> below.  e.g.:
> 
>     for param, value in cert_search_params.viewitems():
>         ...
> 
> ``dict.viewitems()`` returns a *dictview* iterator-like object that
> avoids creating an intermediate list as ``dict.items()`` would.
> 
> 6. Here and in a few other places below it might improve readability to
> test for set membership, e.g.:
> 
>     if param in {
>         'email', 'common_name', 'user'_id', 'org_unit',
>         'org', ...
>     }:
> 
> 7. pycharm appears to be set to 120 columns width by default.  We need
> to set to 80 and reformat accordingly.  Please check in a pycharm
> settings file.  All new code should follow PEP8.
> 
> 8. In CertRevokeRequest, a number of constants are defined for possible
> reason settings.  You should group those, and test for invalid reasons.
> 
> 9.  Do we use CertID anywhere? --- Removed CertID
> 
> 10. list_entrollment_templates has a print r statement in it.  Is that
> supposed to be there?
> 
> 11.  get_enrollment_template should check for None for the profileID.
> 
> 12.  CertRequestInfoCollection has an element - cert_info_list, should
> be
> cert_request_info_list
> 
> 13.  ProfileConstraint -- why is id renamed to name?  Why not just use
> "id"
> -- pycharm and pylint throw a warning when using id as an attribute name
> 
> -- Abhishek
>