[PATCH 2/2] Don't blindly expect that everything is hashable

Anne Mulhern amulhern at redhat.com
Fri Feb 20 14:50:28 UTC 2015





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Friday, February 20, 2015 9:06:57 AM
> Subject: [PATCH 2/2] Don't blindly expect that everything is hashable
> 
> Provide the __hash__ method's for our classes and check that the other
> objects
> are hashable before we try to hash them.
> 
> Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> ---
>  src/parted/alignment.py  | 9 ++++++---
>  src/parted/cachedlist.py | 3 +++
>  src/parted/device.py     | 9 ++++++---
>  src/parted/disk.py       | 9 ++++++---
>  src/parted/filesystem.py | 9 ++++++---
>  src/parted/geometry.py   | 9 ++++++---
>  src/parted/partition.py  | 9 ++++++---
>  7 files changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/src/parted/alignment.py b/src/parted/alignment.py
> index ad059dc..452125e 100644
> --- a/src/parted/alignment.py
> +++ b/src/parted/alignment.py
> @@ -55,12 +55,12 @@ class Alignment(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +

Might make more sense just to get rid of hash() calls in ne
entirely. Previously, all they did was serve as some kind of
optimization on object identity, it looks like.

And Alignment does define hash(), since it extends object.

Also, it's easier to reason about correctness of hash() wrt.
eq if they don't depend on each other.

Why not just make hash return some synthesis of the two fields,
offset and grainSize, that actually mattered to equality in the original?

Or, if you really think that equality should now depend on alignment as well,
(I don't know if that makes sense or not),
make that explicit in the __ne__ method, rather than hiding it in that
change on the hash method.

Generally speaking, it's not really a good idea to make __hash__ be a function
of __str__(), AFAIK.

- mulhern

>          return self.offset != other.offset or self.grainSize !=
>          other.grainSize
>  
>      def __str__(self):
> @@ -71,6 +71,9 @@ class Alignment(object):
>                "ped": self.__alignment})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      @localeC
>      def intersect(self, b):
>          """Create and return a new Alignment that describes the intersection
>          of
> diff --git a/src/parted/cachedlist.py b/src/parted/cachedlist.py
> index 6c522dc..cbf7e39 100644
> --- a/src/parted/cachedlist.py
> +++ b/src/parted/cachedlist.py
> @@ -75,6 +75,9 @@ class CachedList(Sequence):
>          self.__rebuildList()
>          return str(self._lst)
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      def count(self, value):
>          self.__rebuildList()
>          return self._lst.count(value)
> diff --git a/src/parted/device.py b/src/parted/device.py
> index 8cb24fe..5ae2fe7 100644
> --- a/src/parted/device.py
> +++ b/src/parted/device.py
> @@ -59,12 +59,12 @@ class Device(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +
>          return self.model != other.model or self.path != other.path or
>          self.type != other.type or self.length != other.length
>  
>      def __getCHS(self, geometry):
> @@ -182,6 +182,9 @@ class Device(object):
>                "ped": self.__device})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      @localeC
>      def clobber(self):
>          """Remove all identifying signatures of the partition table."""
> diff --git a/src/parted/disk.py b/src/parted/disk.py
> index a450c08..3006266 100644
> --- a/src/parted/disk.py
> +++ b/src/parted/disk.py
> @@ -74,12 +74,12 @@ class Disk(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +
>          return self.device != other.device or not self._hasSameParts(other)
>  
>      def __str__(self):
> @@ -95,6 +95,9 @@ class Disk(object):
>                "ped": self.__disk})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      def __getPartitions(self):
>          """Construct a list of partitions on the disk.  This is called only
>          as
>             needed from the self.partitions property, which just happens to
>             be
> diff --git a/src/parted/filesystem.py b/src/parted/filesystem.py
> index 3c8045c..9478c45 100644
> --- a/src/parted/filesystem.py
> +++ b/src/parted/filesystem.py
> @@ -62,12 +62,12 @@ class FileSystem(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +
>          return self.type != other.type or self.geometry != other.geometry
>  
>      def __str__(self):
> @@ -78,6 +78,9 @@ class FileSystem(object):
>                "checked": self.checked, "ped": self.__fileSystem})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      @property
>      def type(self):
>          """The type of this filesystem, e.g. ext3."""
> diff --git a/src/parted/geometry.py b/src/parted/geometry.py
> index 3316ab9..c391abb 100644
> --- a/src/parted/geometry.py
> +++ b/src/parted/geometry.py
> @@ -67,12 +67,12 @@ class Geometry(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +
>          return self.device != other.device or self.start != other.start or
>          self.length != other.length
>  
>      def __str__(self):
> @@ -83,6 +83,9 @@ class Geometry(object):
>                "device": self.device, "ped": self.__geometry})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      @property
>      def device(self):
>          """The Device this geometry describes."""
> diff --git a/src/parted/partition.py b/src/parted/partition.py
> index 89cb673..a24f9e4 100644
> --- a/src/parted/partition.py
> +++ b/src/parted/partition.py
> @@ -72,12 +72,12 @@ class Partition(object):
>          return not self.__ne__(other)
>  
>      def __ne__(self, other):
> -        if hash(self) == hash(other):
> -            return False
> -
>          if type(self) != type(other):
>              return True
>  
> +        if hasattr(other, "__hash__"):
> +            return hash(self) != hash(other)
> +
>          return self.path != other.path or self.type != other.type or
>          self.geometry != other.geometry or self.fileSystem !=
>          other.fileSystem
>  
>      def __str__(self):
> @@ -97,6 +97,9 @@ class Partition(object):
>                "busy": self.busy, "ped": self.__partition})
>          return s
>  
> +    def __hash__(self):
> +        return hash(str(self))
> +
>      def __writeOnly(self, prop):
>          raise parted.WriteOnlyProperty(prop)
>  
> --
> 2.1.0
> 
> _______________________________________________
> 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