[Pki-devel] [PATCH] 59-2 Fixing for comments given for Patch 59

[Abhishek Koneru]

Addressed all the comments given. Pushed to master.

On Tue, 2013-06-25 at 18:40 -0500, Endi Sukma Dewata wrote:
> On 6/24/2013 4:29 PM, Abhishek Koneru wrote:
> > Please review the patch with the changes suggested by Endi to refactor
> > code inorder to maintain references to the global variables and the
> > utility classes.
> >
> > A new class PKIDeployer is added to pkihelper.py, whose object is
> > created in pkispawn/pkidestroy to hold the references and be passed to
> > the PKIScriptlets.
> >
> > Declarations for pki_master_dict and pki_slots_dict have been moved to
> > init method in pkiparser.py from pkiconfig. The compose methods are
> > called in pkispawn/pkidestroy and then the references passed to the
> > PKIDeployer object.
> >
> > This also fixes many issues of tpe E1120 reported by pylint.
> >
> > --Abhishek
> 
> Some comments:
> 
> 1. The helper class names begin with underscore (e.g. _Identity). I 
> don't think this is necessary. These classes will be used by scriptlet 
> writers, so they aren't meant to be private classes.
> 
> 2. Some of the constructors are defined like this:
> 
>      def __init__(self, pki_master_dict, identity = None):
>          self.master_dict = pki_master_dict
>          if identity is None:
>              self.identity = _Identity(pki_master_dict)
>          else:
>              self.identity = identity
> 
> Since these classes are always used with the deployer object, we can 
> simplify it like this:
> 
>      def __init__(self, deployer):
>          self.master_dict = deployer.pki_master_dict
>          self.identity = deployer.identity
> 
> Using the deployer guarantees that the dicts and helper objects are 
> already created, so there's no need to create the objects again.
> 
> 3. There are some trailing whitespaces.
> 
> Everything else is fine, the code works. With the above issues fixed, ACK.
> 
> 
> As future enhancement, it's possible to simplify the scriptlets further. 
> For example:
> 
>      deployer.file.copy_with_slot_substitution(
>          deployer.master_dict['pki_source_catalina_properties'],
>          deployer.master_dict['pki_target_catalina_properties'],
>          overwrite_flag=True)
> 
> If the method is always used with values from the master_dict, the above 
> line can be simplified as follows:
> 
>      deployer.file.copy_with_slot_substitution(
>          'pki_source_catalina_properties',
>          'pki_target_catalina_properties',
>          overwrite_flag=True)
>