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

[Ade Lee]

Still reviewing, but just a couple of quick notes.

1.  You need to not track workspace.xml - and most likely add it
to .gitignore.  See
http://manski.net/2012/05/which-project-files-under-version-control/#intellij.2C-pycharm.2C-phpstorm.2C-webstorm

That said, it appears that Barbican for example, does not in fact check
in their .idea files.  You might want to ask them about that.

2. In CertRevoke, you do check to see if the reason provided is a valid
reason, but do this by defining the valid reasons twice - once as a
standalone field, and once in an anonymous list.  Better to define all
the reasons in a named list, and check that list.

On Thu, 2014-05-29 at 16:03 -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.
> 
> 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