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

[Fraser Tweedale]

On Mon, Jun 02, 2014 at 11:40:19AM +1000, Fraser Tweedale wrote:
> 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.
> 

One more thing:

4) due to use of ``pki.handle_exceptions`` decorator, docstrings on
decorated methods are wrong.  You can use ``functools.wraps`` to
propagate docstrings through a decorator.  See
http://stackoverflow.com/questions/1782843/python-decorator-problem-with-docstrings

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