[Pki-devel] [PATCH] 95 Fixes for comments for patches 92-2, 93, 94(CertClient and ProfileClient python implementations)
Abhishek Koneru
akoneru at redhat.com
Mon Jun 2 11:08:48 UTC 2014
Comments inline.
On Mon, 2014-06-02 at 11:50 +1000, Fraser Tweedale wrote:
> 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.
The ProfilePolicySet class is being used in the CertReviewResponse
class. It is different from the PolicySet class.
The former is a list of ProfilePolicy objects while the latter is a
dictionary of {Policy_name, Collection of <Profile_Policy>}.
Both classes are replications of similar definitions on the Java side.
> >
> > 2) as currently implemented, ``PolicySet.from_json`` has no explicit
> > return, therefore returns None
> >
> > 3) ``PolicySet.from_json`` likewise lacks a return statement.
> >
Will be corrected.
>
> 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
>
Good catch and great suggestion. I will work on this.
--Abhishek
> > 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
More information about the Pki-devel
mailing list