Repository: pydicom/pydicom

Issue:
PersonName3 behavor is odd because of PersonName3.__hash__ = object.__hash__
Currently here is how PersonName3__hash__ is implemented:
```py
# For python 3, any override of __cmp__ or __eq__
# immutable requires explicit redirect of hash
# function to the parent class See
# See http://docs.python.org/
#  dev/3.0/reference/datamodel.html#object.__hash__
__hash__ = object.__hash__
```
But despite the comment I don't think this is the intended behavior. From my understanding  the comment apply when the parent class is an immutable, Because `object` is mutable PersonName3 doesn't behave like an immutable.

examples:
```py
In [10]: p1 = PersonName3("Foo")
In [11]: p2 = PersonName3("Foo")

In [12]: p1 == p2
Out[12]: True

In [13]: p1.__hash__() == p2.__hash__()
Out[13]: False # Oups!

In [14]: d = {}
In [15]: d[p1] = 1
In [16]: d[p2] = 2

In [17]: d
Out[17]: {'Foo': 1, 'Foo': 2}
```
What you probably want is something like:

```py
def __hash__(self):
    return hash(tuple(self.components)
```

Also it would make more sense IMO to return components as a `tuple` rather than a `list`

Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for submitting the PR.

 - In the file [`pydicom/tests/test_valuerep.py`](https://github.com/mrbean-bremen/pydicom/blob/4b2ad7012c5dd3f1c65659b0a30d65fbf32dd39e/pydicom/tests/test_valuerep.py), following                are the PEP8 issues :
> [Line 386:32](https://github.com/mrbean-bremen/pydicom/blob/4b2ad7012c5dd3f1c65659b0a30d65fbf32dd39e/pydicom/tests/test_valuerep.py#L386): [E128](https://duckduckgo.com/?q=pep8%20E128) continuation line under-indented for visual indent
> [Line 388:32](https://github.com/mrbean-bremen/pydicom/blob/4b2ad7012c5dd3f1c65659b0a30d65fbf32dd39e/pydicom/tests/test_valuerep.py#L388): [E128](https://duckduckgo.com/?q=pep8%20E128) continuation line under-indented for visual indent
> [Line 393:1](https://github.com/mrbean-bremen/pydicom/blob/4b2ad7012c5dd3f1c65659b0a30d65fbf32dd39e/pydicom/tests/test_valuerep.py#L393): [E303](https://duckduckgo.com/?q=pep8%20E303) too many blank lines (3)


 - There are no PEP8 issues in the file [`pydicom/valuerep.py`](https://github.com/mrbean-bremen/pydicom/blob/4b2ad7012c5dd3f1c65659b0a30d65fbf32dd39e/pydicom/valuerep.py) !



Patch:
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index 5daceb616..48ebac8a4 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -23,3 +23,5 @@ Fixes
   in explicit transfer syntax (:issue:`757`)
 * Do not derive `Dataset` from `dict` - fixes behavior in newer Python versions
   (:issue:`765`)
+* Make hash for `PersonName3` behave as expected, make `PersonName` objects
+  immutable (:issue:`785`)
diff --git a/pydicom/valuerep.py b/pydicom/valuerep.py
index b5f2b7a72..49e857802 100644
--- a/pydicom/valuerep.py
+++ b/pydicom/valuerep.py
@@ -531,9 +531,10 @@ def MultiString(val, valtype=str):
 
 def _verify_encodings(encodings):
     """Checks the encoding to ensure proper format"""
-    if encodings is not None and not isinstance(encodings, list):
-        return [encodings]
-
+    if encodings is not None:
+        if not isinstance(encodings, (list, tuple)):
+            return encodings,
+        return tuple(encodings)
     return encodings
 
 
@@ -565,7 +566,7 @@ def _decode_personname(components, encodings):
     # Remove empty elements from the end to avoid trailing '='
     while len(comps) and not comps[-1]:
         comps.pop()
-    return comps
+    return tuple(comps)
 
 
 def _encode_personname(components, encodings):
@@ -607,7 +608,7 @@ class PersonName3(object):
         if isinstance(val, PersonName3):
             encodings = val.encodings
             self.original_string = val.original_string
-            self._components = str(val).split('=')
+            self._components = tuple(str(val).split('='))
         elif isinstance(val, bytes):
             # this is the raw byte string - decode it on demand
             self.original_string = val
@@ -616,7 +617,7 @@ class PersonName3(object):
             # this is the decoded string - save the original string if
             # available for easier writing back
             self.original_string = original_string
-            self._components = val.split('=')
+            self._components = tuple(val.split('='))
 
         # if the encoding is not given, leave it as undefined (None)
         self.encodings = _verify_encodings(encodings)
@@ -710,12 +711,8 @@ class PersonName3(object):
     def __repr__(self):
         return '='.join(self.components).__repr__()
 
-    # For python 3, any override of __cmp__ or __eq__
-    # immutable requires explicit redirect of hash
-    # function to the parent class See
-    # See http://docs.python.org/
-    #  dev/3.0/reference/datamodel.html#object.__hash__
-    __hash__ = object.__hash__
+    def __hash__(self):
+        return hash(self.components)
 
     def decode(self, encodings=None):
         """Return the patient name decoded by the given encodings.
@@ -807,7 +804,7 @@ class PersonNameBase(object):
 
     def parse(self):
         """Break down the components and name parts"""
-        self.components = self.split("=")
+        self.components = tuple(self.split("="))
         nComponents = len(self.components)
         self.single_byte = self.components[0]
         self.ideographic = ''
@@ -903,8 +900,8 @@ class PersonNameUnicode(PersonNameBase, compat.text_type):
         name = compat.text_type(self).encode('utf8')
         new_person = PersonNameUnicode(name, 'utf8')
         memo[id(self)] = new_person
-        for k, v in self.__dict__.items():
-            setattr(new_person, k, deepcopy(v, memo))
+        # no need for deepcopy call - all attributes are immutable
+        new_person.__dict__.update(self.__dict__)
         return new_person
 
     def encode(self, encodings):


Test patch:
diff --git a/pydicom/tests/test_valuerep.py b/pydicom/tests/test_valuerep.py
index 4aa1cbae9..5f1efd348 100644
--- a/pydicom/tests/test_valuerep.py
+++ b/pydicom/tests/test_valuerep.py
@@ -278,9 +278,9 @@ class TestPersonName(object):
         pn_copy = copy.deepcopy(pn)
         assert pn == pn_copy
         assert pn.components == pn_copy.components
-        # deepcopy() shall have made a copy of components
-        assert pn_copy.components is not pn.components
-        assert pn.encodings == pn_copy.encodings
+        # deepcopy() returns the same immutable objects (tuples)
+        assert pn_copy.components is pn.components
+        assert pn.encodings is pn_copy.encodings
 
     def test_three_component(self):
         """PN: 3component (single-byte, ideographic,
@@ -369,9 +369,24 @@ class TestPersonName(object):
         # Issue 466
         from pydicom.valuerep import PersonName3
         pn = PersonName3("John^Doe", encodings='iso_ir_126')
-        assert pn.encodings == ['iso_ir_126']
+        assert pn.encodings == ('iso_ir_126',)
         pn2 = PersonName3(pn)
-        assert pn2.encodings == ['iso_ir_126']
+        assert pn2.encodings == ('iso_ir_126',)
+
+    def test_hash(self):
+        """Test that the same name creates the same hash."""
+        # Regression test for #785 in Python 3
+        pn1 = PersonNameUnicode("John^Doe^^Dr", encodings=default_encoding)
+        pn2 = PersonNameUnicode("John^Doe^^Dr", encodings=default_encoding)
+        assert hash(pn1) == hash(pn2)
+        pn3 = PersonNameUnicode("John^Doe", encodings=default_encoding)
+        assert hash(pn1) != hash(pn3)
+
+        pn1 = PersonNameUnicode(u'Yamada^Tarou=山田^太郎=やまだ^たろう',
+                                [default_encoding, 'iso2022_jp'])
+        pn2 = PersonNameUnicode(u'Yamada^Tarou=山田^太郎=やまだ^たろう',
+                                [default_encoding, 'iso2022_jp'])
+        assert hash(pn1) == hash(pn2)
 
 
 class TestDateTime(object):