[PATCH] Don't use OrderedDict.

Anne Mulhern amulhern at redhat.com
Tue Nov 12 20:05:13 UTC 2013





----- Original Message -----
> From: "Chris Lumens" <clumens at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Tuesday, November 12, 2013 2:29:15 PM
> Subject: [PATCH] Don't use OrderedDict.
> 
> This doesn't exist in the python shipped with RHEL6, and pykickstart is
> still being used on those systems for testing purposes.
> ---
>  pykickstart/commands/autopart.py | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/pykickstart/commands/autopart.py
> b/pykickstart/commands/autopart.py
> index 089788c..3f39793 100644
> --- a/pykickstart/commands/autopart.py
> +++ b/pykickstart/commands/autopart.py
> @@ -21,8 +21,6 @@ from pykickstart.base import *
>  from pykickstart.errors import *
>  from pykickstart.options import *
>  
> -from collections import OrderedDict
> -
>  import gettext
>  _ = lambda x: gettext.ldgettext("pykickstart", x)
>  
> @@ -215,28 +213,28 @@ class F17_AutoPart(F16_AutoPart):
>      def __init__(self, writePriority=100, *args, **kwargs):
>          F16_AutoPart.__init__(self, writePriority=writePriority, *args,
>          **kwargs)
>          self.type = kwargs.get("type", None)
> -        # This uses OrderedDict because we want to always output
> --type=plain
> -        # (as opposed to --type=partition) in __str__ when self.type is
> -        # AUTOPART_TYPE_PLAIN.
> -        self.typeMap = OrderedDict([("lvm", AUTOPART_TYPE_LVM),
> -                                    ("btrfs", AUTOPART_TYPE_BTRFS),
> -                                    ("plain", AUTOPART_TYPE_PLAIN),
> -                                    ("partition", AUTOPART_TYPE_PLAIN)])
> +        self.typeMap = { "lvm": AUTOPART_TYPE_LVM,
> +                         "btrfs": AUTOPART_TYPE_BTRFS,
> +                         "plain": AUTOPART_TYPE_PLAIN,
> +                         "partition": AUTOPART_TYPE_PLAIN }
> +
> +    def _typeAsStr(self):
> +        if self.type == AUTOPART_TYPE_LVM:
> +            return "lvm"
> +        elif self.type == AUTOPART_TYPE_BTRFS:
> +            return "btrfs"
> +        elif self.type == AUTOPART_TYPE_PLAIN:
> +            return "plain"
>  
>      def __str__(self):
>          retval = F16_AutoPart.__str__(self)
>          if not self.autopart:
>              return retval
>  
> -        if self.type is not None:
> +        if self.type in self.typeMap.values():
>              # remove any trailing newline
>              retval = retval.strip()
> -            retval += " --type="
> -            for s, n in self.typeMap.items():
> -                if self.type == n:
> -                    retval += s
> -                    break
> -            retval += "\n"
> +            retval += " --type=%s\n" % self._typeAsStr()
>  
>          return retval
>  
> @@ -301,6 +299,12 @@ class F20_AutoPart(F18_AutoPart):
>          F18_AutoPart.__init__(self, writePriority=writePriority, *args,
>          **kwargs)
>          self.typeMap["thinp"] = AUTOPART_TYPE_LVM_THINP
>  
> +    def _typeAsStr(self):
> +        if self.type == AUTOPART_TYPE_LVM_THINP:
> +            return "thinp"
> +        else:
> +            return F18_AutoPart._typeAsStr(self)
> +
>      def parse(self, args):
>          # call the overriden command to do it's job first
>          retval = F18_AutoPart.parse(self, args)
> --
> 1.8.3.1

For _typeAsStr
Maybe it would be better to have a strategy that went through all the items in self.typeMap.
If it found more than one match it would break the tie in some appropriate way and return the correct one.
If it found none it would return None.
If it coudn't break the tie it would panic or just guess and log.

Then you could use it like:
type_as_str = self._typeAsStr()
if type_as_str:
    retval += " --type=%s\n" % type_as_str

You could avoid the overload in F20 and the possibility that a buggy overload of _typeAsStr would
return None even if the lookup value was in typeMap.values().

It feels like this implementation would be more robust, especially to extension.

- mulhern

> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list