[Pki-devel] [PATCH] 95 -2 Fixes for comments on patch 95

[Abhishek Koneru]

So i addressed all the new comments from Fraser and Ade.
Ade is OK with the current set of patches checked in (93, 94, 95-2).
I will be submitting new patches which add the remaining methods in
ProfileClient and provide a better solution for the property/setter
usage in classes like ProfileData, CertEnrollmentRequest and
CertReviewResponse.

Fraser - if the current code is OK with you please let me know. i can
check in the code right away.

As per my previous mail - these are addressed:

> > > 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.
Though the link you provided is pretty old, i still do not see any
support for showing the arguments of the wrapped function instead of the
wrapper function's arguments - when calling help.
In any case, i used functools.wraps to atleast show the docstring.

Along with the following comments by Ade.

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.

I removed the PyCharm project files. (Even barbican doesn't check them
in and they do not have any other way of checking the PEP8 standards
using the PyCharm files)

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.

-- Included in the patch.

-- Abhishek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-95-2-Addressed-comments-given-for-patches-92-2-93-94.patch
Type: text/x-patch
Size: 65853 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140602/439ba413/attachment.bin>