6:08 a.m.
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-...
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel