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

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