Re: [Pki-devel] [PATCH] 59-2 Fixing for comments given for Patch 59
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)
--
Endi S. Dewata