Repository: pydicom/pydicom

Issue:
DA class is inconsistent
**Describe the bug**
pydicom.valuerep.DA accepts strings or datetime.date objects - but DA objects created with datetime.date inputs are invalid. 

**Expected behavior**
I would expect both of these expressions to generate the same output:
```
print(f'DA("20201117") => {DA("20201117")}')
print(f'DA(date(2020, 11, 17)) => {DA(date(2020, 11, 17))}')
```
but instead I get
```
DA("20201117") => 20201117
DA(date(2020, 11, 17)) => 2020-11-17
```
The hyphens inserted into the output are not valid DICOM - see the DA description in [Table 6.2-1](http://dicom.nema.org/dicom/2013/output/chtml/part05/sect_6.2.html)

**Steps To Reproduce**
Run the following commands:
```
from pydicom.valuerep import DA
from pydicom.dataset import Dataset
from datetime import date, datetime

print(f'DA("20201117") => {DA("20201117")}')
print(f'DA(date(2020, 11, 17)) => {DA(date(2020, 11, 17))}')

# 1. JSON serialization with formatted string works
ds = Dataset()
ds.ContentDate = '20201117'
json_output = ds.to_json()
print(f'json_output works = {json_output}')

# 2. JSON serialization with date object input is invalid.
ds = Dataset()
ds.ContentDate = str(DA(date(2020, 11, 17)))
json_output = ds.to_json()
print(f'json_output with str(DA..) - invalid DICOM {json_output}')

# 3. JSON serialization with date object fails
ds = Dataset()
ds.ContentDate = DA(date(2020, 11, 17))

# Exception on this line: TypeError: Object of type DA is not JSON serializable
json_output = ds.to_json()

```

I believe that all three approaches should work - but only the first is valid. The method signature on DA's `__new__` method accepts datetime.date objects. 

**Your environment**
```
module       | version
------       | -------
platform     | macOS-10.15.7-x86_64-i386-64bit
Python       | 3.8.6 (default, Oct  8 2020, 14:06:32)  [Clang 12.0.0 (clang-1200.0.32.2)]
pydicom      | 2.1.0
gdcm         | _module not found_
jpeg_ls      | _module not found_
numpy        | 1.19.4
PIL          | 8.0.1
```



Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) Report
> Merging [#1365](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) (7e8f362) into [master](https://codecov.io/gh/pydicom/pydicom/commit/bd82f01faf4212f6e43f55a1cc6da17956122d8f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) (bd82f01) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head 7e8f362 differs from pull request most recent head df2f9f1. Consider uploading reports for the commit df2f9f1 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/pydicom/pydicom/pull/1365/graphs/tree.svg?width=650&height=150&src=pr&token=YciLV15Lwr&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom)](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom)

```diff
@@           Coverage Diff           @@
##           master    #1365   +/-   ##
=======================================
  Coverage   95.23%   95.24%           
=======================================
  Files          60       60           
  Lines        9704     9720   +16     
=======================================
+ Hits         9242     9258   +16     
  Misses        462      462           
```


| [Impacted Files](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) | Coverage Δ | |
|---|---|---|
| [pydicom/valuerep.py](https://codecov.io/gh/pydicom/pydicom/pull/1365/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom#diff-cHlkaWNvbS92YWx1ZXJlcC5weQ==) | `98.32% <100.00%> (-0.16%)` | :arrow_down: |
| [pydicom/filewriter.py](https://codecov.io/gh/pydicom/pydicom/pull/1365/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom#diff-cHlkaWNvbS9maWxld3JpdGVyLnB5) | `98.19% <0.00%> (+0.25%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom). Last update [bd82f01...df2f9f1](https://codecov.io/gh/pydicom/pydicom/pull/1365?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom).

scaramallion: Hmm, actually could you add a couple of tests to make sure it still works OK after pickling?
mrbean-bremen: > Hmm, actually could you add a couple of tests to make sure it still works OK after pickling?

Yes, good point, will do.
mrbean-bremen: Turns out that pickling a DT with a timezone does not work (independent of my changes). I haven't understood it yet, but I think I have to fix this first (maybe tomorrow...).
mrbean-bremen: Hm, I'm still struggling with the pickle problem - at the moment the only way to get it working is to not derive `DT` from `datetime.datetime`, but use composition instead. If deriving from `datetime.datetime` as is, the timezone is not pickled, and I currently see no way to get it to work.
If I want to change it that way, I'll probably also change `DA` and `TM`, and shall add more tests to make sure the current behavior is not changed, but I'm not sure if this is a good idea...

Actually I'll probably handle this in a separate issue, as it has nothing to do with the current PR and turned out to be more complicated than I thought. I will add tests for pickling for that issue.

Patch:
diff --git a/doc/release_notes/v2.2.0.rst b/doc/release_notes/v2.2.0.rst
index 10844ca90..72fb9b9ca 100644
--- a/doc/release_notes/v2.2.0.rst
+++ b/doc/release_notes/v2.2.0.rst
@@ -65,6 +65,11 @@ Changes
 * :meth:`BaseTag.__eq__()<pydicom.tag.BaseTag.__eq__>` returns ``False`` rather
   than raising an exception when the operand cannot be converted to
   :class:`~pydicom.tag.BaseTag` (:pr:`1327`)
+* :meth:`DA.__str__()<pydicom.valuerep.DA.__str__>`,
+  :meth:`DT.__str__()<pydicom.valuerep.DT.__str__>` and
+  :meth:`TM.__str__()<pydicom.valuerep.TM.__str__>` return valid DICOM
+  strings instead of the formatted date and time representations
+  (:issue:`1262`)
 
 Fixes
 -----
diff --git a/pydicom/valuerep.py b/pydicom/valuerep.py
index fb0840841..a4acdb114 100644
--- a/pydicom/valuerep.py
+++ b/pydicom/valuerep.py
@@ -135,6 +135,8 @@ class DA(_DateTimeBase, datetime.date):
             self.original_string = val
         elif isinstance(val, DA) and hasattr(val, 'original_string'):
             self.original_string = val.original_string
+        elif isinstance(val, datetime.date):
+            self.original_string = f"{val.year}{val.month:02}{val.day:02}"
 
 
 class DT(_DateTimeBase, datetime.datetime):
@@ -242,6 +244,22 @@ class DT(_DateTimeBase, datetime.datetime):
             self.original_string = val
         elif isinstance(val, DT) and hasattr(val, 'original_string'):
             self.original_string = val.original_string
+        elif isinstance(val, datetime.datetime):
+            self.original_string = (
+                f"{val.year:04}{val.month:02}{val.day:02}"
+                f"{val.hour:02}{val.minute:02}{val.second:02}"
+            )
+            # milliseconds are seldom used, add them only if needed
+            if val.microsecond > 0:
+                self.original_string += f".{val.microsecond:06}"
+            if val.tzinfo is not None:
+                offset = val.tzinfo.utcoffset(val)
+                offset_min = offset.days * 24 * 60 + offset.seconds // 60
+                sign = "+" if offset_min >= 0 else "-"
+                offset_min = abs(offset_min)
+                self.original_string += (
+                    f"{sign}{offset_min // 60:02}{offset_min % 60:02}"
+                )
 
 
 class TM(_DateTimeBase, datetime.time):
@@ -318,6 +336,13 @@ class TM(_DateTimeBase, datetime.time):
             self.original_string = val
         elif isinstance(val, TM) and hasattr(val, 'original_string'):
             self.original_string = val.original_string
+        elif isinstance(val, datetime.time):
+            self.original_string = (
+                f"{val.hour:02}{val.minute:02}{val.second:02}"
+            )
+            # milliseconds are seldom used, add them only if needed
+            if val.microsecond > 0:
+                self.original_string += f".{val.microsecond:06}"
 
     if platform.python_implementation() == "PyPy":
         # Workaround for CPython/PyPy bug in time.__reduce_ex__()


Test patch:
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index 0bbeab82b..5052d8a9b 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -300,8 +300,7 @@ class TestScratchWriteDateTime(TestWriteFile):
         DA_expected = date(1961, 8, 4)
         tzinfo = timezone(timedelta(seconds=-21600), '-0600')
         multi_DT_expected = (datetime(1961, 8, 4), datetime(
-            1963, 11, 22, 12, 30, 0, 0,
-            timezone(timedelta(seconds=-21600), '-0600')))
+            1963, 11, 22, 12, 30, 0, 0, tzinfo))
         multi_TM_expected = (time(1, 23, 45), time(11, 11, 11))
         TM_expected = time(11, 11, 11, 1)
         ds = dcmread(datetime_name)
diff --git a/pydicom/tests/test_valuerep.py b/pydicom/tests/test_valuerep.py
index e84deb2f1..9268d649c 100644
--- a/pydicom/tests/test_valuerep.py
+++ b/pydicom/tests/test_valuerep.py
@@ -69,11 +69,15 @@ class TestTM:
 
     def test_str(self):
         """Test str(TM)."""
-        x = pydicom.valuerep.TM("212223")
-        assert "212223" == str(x)
-        del x.original_string
-        assert not hasattr(x, 'original_string')
-        assert "21:22:23" == str(x)
+        assert "212223.1234" == str(pydicom.valuerep.TM("212223.1234"))
+        assert "212223" == str(pydicom.valuerep.TM("212223"))
+        assert "212223" == str(pydicom.valuerep.TM("212223"))
+        assert "2122" == str(pydicom.valuerep.TM("2122"))
+        assert "21" == str(pydicom.valuerep.TM("21"))
+        assert "212223" == str(pydicom.valuerep.TM(time(21, 22, 23)))
+        assert "212223.000024" == str(
+            pydicom.valuerep.TM(time(21, 22, 23, 24)))
+        assert "010203" == str(pydicom.valuerep.TM(time(1, 2, 3)))
 
     def test_new_empty_str(self):
         """Test converting an empty string."""
@@ -185,6 +189,18 @@ class TestDT:
         with pytest.raises(ValueError, match=msg):
             pydicom.valuerep.DT("a2000,00,00")
 
+    def test_str(self):
+        dt = datetime(1911, 12, 13, 21, 21, 23)
+        assert "19111213212123" == str(pydicom.valuerep.DT(dt))
+        assert "19111213212123" == str(pydicom.valuerep.DT("19111213212123"))
+        assert "1001.02.03" == str(pydicom.valuerep.DA("1001.02.03"))
+        tz_info = timezone(timedelta(seconds=21600), '+0600')
+        dt = datetime(2022, 1, 2, 8, 9, 7, 123456, tzinfo=tz_info)
+        assert "20220102080907.123456+0600" == str(pydicom.valuerep.DT(dt))
+        tz_info = timezone(timedelta(seconds=-23400), '-0630')
+        dt = datetime(2022, 12, 31, 23, 59, 59, 42, tzinfo=tz_info)
+        assert "20221231235959.000042-0630" == str(pydicom.valuerep.DT(dt))
+
 
 class TestDA:
     """Unit tests for pickling DA"""
@@ -213,6 +229,11 @@ class TestDA:
         with pytest.raises(ValueError, match=msg):
             pydicom.valuerep.DA(123456)
 
+    def test_str(self):
+        assert "10010203" == str(pydicom.valuerep.DA(date(1001, 2, 3)))
+        assert "10010203" == str(pydicom.valuerep.DA("10010203"))
+        assert "1001.02.03" == str(pydicom.valuerep.DA("1001.02.03"))
+
 
 class TestIsValidDS:
     """Unit tests for the is_valid_ds function."""