[Pki-devel] [PATCH] 97 Implemented remaining methods in the python ProfileClient API

Ade Lee alee at redhat.com
Mon Jun 23 18:56:37 UTC 2014


In general, patch looks good.

1. Are the parens a PEP8/ pycharm/ pylint thing?  If not, I'm not sure I
see the point of adding them.
2. profile.py needs the standard RH copyright notice at the top.
3. I'd like to see a little more testing in the main().  In particular,
I'd like to see the use of Profile Outputs and ProfilePolicySets.

Ade

On Wed, 2014-06-18 at 17:50 +1000, Fraser Tweedale wrote:
> On Wed, Jun 18, 2014 at 01:17:23AM -0400, Abhishek Koneru wrote:
> > Please review the patch which implements the remaining of the 
> > ProfileClient API for create/modify/delete profiles.
> > 
> > Includes basic test cases in the main method of profile.py module.
> > 
> > -- Abhishek
> 
> Applied the patch and ran the main method to test.  Does what it
> says on the tin, but haven't rigorously tested it / used it in anger
> yet.
> 
> Patch looks fine, just one nitpick (see below).
> 
> Cheers,
> 
> Fraser
> 
> > diff --git a/base/common/python/pki/__init__.py b/base/common/python/pki/__init__.py
> > index 891d6ea6364b037f132ff3754b73b372c638b0f7..e9b726cf763785b4a700ef314ff27774b13aba40 100644
> > --- a/base/common/python/pki/__init__.py
> > +++ b/base/common/python/pki/__init__.py
> > @@ -168,7 +168,7 @@ class PKIException(Exception, ResourceMessage):
> >          ret = cls(json_value['Message'], json_value['Code'],
> >                    json_value['ClassName'])
> >          for attr in json_value['Attributes']['Attribute']:
> > -            print str(attr)
> > +            print(str(attr))
> >              ret.add_attribute(attr["name"], attr["value"])
> >          return ret
> >  
> > @@ -299,7 +299,7 @@ class PropertyFile(object):
> >      def show(self):
> >          """ Show contents of property file."""
> >          for line in self.lines:
> > -            print line
> > +            print(line)
> >
> 
> Nit-pick: Why the parens?  Even if this is the correct style, it is
> unrelated to the other changes so might be better to include in a
> separate patch?
> 
> _______________________________________________
> 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