On Fri, Mar 25, 2011 at 10:33, Rene Nussbaumer <[email protected]> wrote:
> On Thu, Mar 24, 2011 at 5:32 PM, Stephen Shirley <[email protected]> wrote:
>> # R0201: Method could be a function
>> + def _MergeClusterConfigs(self, my_config, other_config):
>> + """Checks that all relevant cluster parameters are compatible"""
>> + # pylint: disable-msg=R0201
>
> Put the # pylint comment on the same line as the function signature.
Then lint will barf because the line is >80 chars.. Last time i hit
this issue, hansmi said to just put it after the docstring.
> Wonder if it wouldn't be easier to make it a staticmethod.
We have a nice pattern going now, why break it? :)
> The docstring should look like:
>
> """Checks that all relevant cluster parameters are compatible.
>
> """
Done.
>> + check_params = [
>> + "beparams",
>> + "default_iallocator",
>> + "drbd_usermode_helper",
>> + "file_storage_dir",
>> + "hidden_os",
>> + "maintain_node_health",
>> + "master_netdev",
>> + "ndparams",
>> + "nicparams",
>> + "primary_ip_family",
>> + "tags",
>> + "uid_pool",
>> + "volume_group_name",
>> + ]
>
> As you're just using this list down in the for loop, how about a tuple?
Done.
>> + # Check os hypervisor params for hypervisors we care about
>> + for os_name in set(my_cluster.os_hvp.keys() +
>> other_cluster.os_hvp.keys()):
>> + def _get_os_hvp(cluster, os_name, hyp):
>
> This function doesn't need to be a closure, can you please move it
> outside? Also the signature is not according to the style guide:
> _GetOsHypervisor
It's not a closure :P (it doesn't use any variables from outside its
scope). Done and done.
>> + if os_name in cluster.os_hvp:
>> + return cluster.os_hvp[os_name].get(hyp)
>
> Are you using get here to set the default value in case it doesn't
> exist? Then it's probably missing, otherwise it's still raising a
> KeyError.
>>> {1:1}.get(2)
>>>
get() on a dict for a value it doesn't contain will return None.
Updated patch on its way.
Steve
