Repository: pydicom/pydicom

Issue:
Deprecate DicomDir?
Now that the `FileSet` is in, should we deprecate `DicomDir` and the `dicomdir` module?

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1212?src=pr&el=h1) Report
> Merging [#1212](https://codecov.io/gh/pydicom/pydicom/pull/1212?src=pr&el=desc) into [master](https://codecov.io/gh/pydicom/pydicom/commit/a289cd2bc038afd720d63388f1d31b1b5b95e0c9?el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pydicom/pydicom/pull/1212/graphs/tree.svg?width=650&height=150&src=pr&token=YciLV15Lwr)](https://codecov.io/gh/pydicom/pydicom/pull/1212?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
+ Coverage   94.85%   94.88%   +0.03%     
==========================================
  Files          55       55              
  Lines        9212     9214       +2     
==========================================
+ Hits         8738     8743       +5     
+ Misses        474      471       -3     
```


| [Impacted Files](https://codecov.io/gh/pydicom/pydicom/pull/1212?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pydicom/dicomdir.py](https://codecov.io/gh/pydicom/pydicom/pull/1212/diff?src=pr&el=tree#diff-cHlkaWNvbS9kaWNvbWRpci5weQ==) | `100.00% <ø> (ø)` | |
| [pydicom/filereader.py](https://codecov.io/gh/pydicom/pydicom/pull/1212/diff?src=pr&el=tree#diff-cHlkaWNvbS9maWxlcmVhZGVyLnB5) | `93.83% <100.00%> (+0.84%)` | :arrow_up: |

------

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


Patch:
diff --git a/doc/index.rst b/doc/index.rst
index 40fd4d6d8..f23998f3e 100644
--- a/doc/index.rst
+++ b/doc/index.rst
@@ -44,7 +44,8 @@ Getting Started
 If you're new to *pydicom* then start here:
 
 * :doc:`Installation<tutorials/installation>` |
-  :doc:`Introduction to pydicom<old/getting_started>`
+  :doc:`Introduction to pydicom<old/getting_started>` |
+  :doc:`What Python types do I use for each VR<guides/element_value_types>`
 * **Basics**: :doc:`Read, access, modify, write</tutorials/dataset_basics>`
 * **Intermediate**: :doc:`Waveform decoding and encoding</tutorials/waveforms>`
   | :doc:`DICOM File-sets and DICOMDIR</tutorials/filesets>`
diff --git a/doc/release_notes/v2.1.0.rst b/doc/release_notes/v2.1.0.rst
index c764f3d84..785b4a45d 100644
--- a/doc/release_notes/v2.1.0.rst
+++ b/doc/release_notes/v2.1.0.rst
@@ -18,7 +18,8 @@ Enhancements
   * To download an individual file and get its path on disk you can use
     :func:`pydicom.data.get_testdata_file`, e.g. for ``RG1_UNCI.dcm`` use
     ``pydicom.data.get_testdata_file("RG1_UNCI.dcm")``
-* Added a new pixel data handler based on pylibjpeg which supports all
+* Added a new pixel data handler based on `pylibjpeg
+  <https://github.com/pydicom/pylibjpeg>`_ which supports all
   (non-retired) JPEG transfer syntaxes (:issue:`1127`)
 * Added :func:`~pydicom.pixel_data_handlers.apply_rescale`,
   :func:`~pydicom.pixel_data_handlers.apply_windowing`  aliases
@@ -61,6 +62,10 @@ Changes
 * Reading and adding unknown non-private tags now does not raise an exception
   per default, only when :attr:`~pydicom.config.enforce_valid_values` is set
   (:issue:`1161`)
+* Invalid values used with the :class:`~pydicom.dataset.Dataset`
+  :func:`in<operator.__contains__>` operator now raise :class:`ValueError`
+  rather than returning ``False``, if you need the old behavior use
+  :func:`hasattr` instead (:issue:`1200`)
 * Data dictionaries updated to version 2020d of the DICOM Standard
 * Updated a handful of the SOP Class variable names in
   ``_storage_sopclass_uids``
@@ -68,16 +73,15 @@ Changes
   become ``MultiFrame``, those with ``and`` in them become ``And``, and
   ``DICOSQuadrupoleResonanceQRStorage`` becomes
   ``DICOSQuadrupoleResonanceStorage``.
-* Data dictionaries updated to version 2020c of the DICOM Standard
 * The following UID constants are deprecated and will be removed in v2.2:
 
-  * ``JPEGBaseline`` (use :attr:`~pydicom.uid.JPEGBaseline8Bit` instead)
-  * ``JPEGExtended`` (use :attr:`~pydicom.uid.JPEGExtended12Bit` instead)
-  * ``JPEGLossless`` (use :attr:`~pydicom.uid.JPEGLosslessSV1` instead)
-  * ``JPEGLSLossy`` (use :attr:`~pydicom.uid.JPEGLSNearLossless` instead)
-  * ``JPEG2000MultiComponentLossless`` (use
-    :attr:`~pydicom.uid.JPEG2000MCLossless` instead)
-  * ``JPEG2000MultiComponent`` (use :attr:`~pydicom.uid.JPEG2000MC` instead)
+  * ``JPEGBaseline``: use :attr:`~pydicom.uid.JPEGBaseline8Bit`
+  * ``JPEGExtended``: use :attr:`~pydicom.uid.JPEGExtended12Bit`
+  * ``JPEGLossless``: use :attr:`~pydicom.uid.JPEGLosslessSV1`
+  * ``JPEGLSLossy``: use :attr:`~pydicom.uid.JPEGLSNearLossless`
+  * ``JPEG2000MultiComponentLossless``: use
+    :attr:`~pydicom.uid.JPEG2000MCLossless`
+  * ``JPEG2000MultiComponent``: use :attr:`~pydicom.uid.JPEG2000MC`
 
 * In v3.0 the value for ``JPEGLossless`` will change from
   1.2.840.10008.1.2.4.70 to 1.2.840.10008.1.2.4.57 to match its UID keyword
@@ -94,6 +98,9 @@ Changes
   * ``UncompressedPixelTransferSyntaxes``: use
     :attr:`~pydicom.uid.UncompressedTransferSyntaxes`
   * ``PILSupportedCompressedPixelTransferSyntaxes``
+* :class:`~pydicom.dicomdir.DicomDir` and the ``dicomdir`` module are
+  deprecated and will be removed in v3.0. Use :class:`~pydicom.fileset.FileSet`
+  instead (:issue:`1211`)
 
 Fixes
 .....
diff --git a/pydicom/dicomdir.py b/pydicom/dicomdir.py
index 4e6b55035..c70802270 100644
--- a/pydicom/dicomdir.py
+++ b/pydicom/dicomdir.py
@@ -12,6 +12,11 @@ class DicomDir(FileDataset):
 
     Derived from :class:`~pydicom.dataset.FileDataset`, but additional methods
     are available, specific to the Directory structure
+
+    .. deprecated:: 2.1
+
+        ``DicomDir`` is deprecated and will be removed in *pydicom* v3.0,
+        use :class:`~pydicom.fileset.FileSet` instead.
     """
 
     def __init__(self,
diff --git a/pydicom/filereader.py b/pydicom/filereader.py
index ff0c5ebd0..711467348 100644
--- a/pydicom/filereader.py
+++ b/pydicom/filereader.py
@@ -752,6 +752,12 @@ def read_partial(fileobj, stop_when=None, defer_size=None,
 
     class_uid = file_meta_dataset.get("MediaStorageSOPClassUID", None)
     if class_uid and class_uid.name == "Media Storage Directory Storage":
+        warnings.warn(
+            "The 'DicomDir' class is deprecated and will be removed in v3.0, "
+            "after which 'dcmread()' will return a normal 'FileDataset' "
+            "instance for 'Media Storage Directory' SOP Instances.",
+            DeprecationWarning
+        )
         dataset_class = DicomDir
     else:
         dataset_class = FileDataset
@@ -886,6 +892,11 @@ def read_dicomdir(filename="DICOMDIR"):
 
     This is a wrapper around :func:`dcmread` which gives a default file name.
 
+    .. deprecated:: 2.1
+
+        ``read_dicomdir()`` is deprecated and will be removed in v3.0. Use
+        :func:`~pydicom.filereader.dcmread` instead.
+
     Parameters
     ----------
     filename : str, optional
@@ -901,6 +912,11 @@ def read_dicomdir(filename="DICOMDIR"):
         Raised if filename is not a DICOMDIR file.
     """
     # dcmread will return a DicomDir instance if file is one.
+    warnings.warn(
+        "'read_dicomdir()' is deprecated and will be removed in v3.0, use "
+        "'dcmread()' instead",
+        DeprecationWarning
+    )
 
     # Read the file as usual.
     ds = dcmread(filename)


Test patch:
diff --git a/pydicom/tests/test_dicomdir.py b/pydicom/tests/test_dicomdir.py
index 9226e0ffe..be9e67c0b 100644
--- a/pydicom/tests/test_dicomdir.py
+++ b/pydicom/tests/test_dicomdir.py
@@ -19,6 +19,7 @@ TEST_FILES = (
 )
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestDicomDir:
     """Test dicomdir.DicomDir class"""
 
@@ -72,3 +73,13 @@ class TestDicomDir:
         with pytest.raises(InvalidDicomError,
                            match='Invalid transfer syntax*'):
             dcmread(BIGENDIAN_TEST_FILE)
+
+
+def test_deprecation_warning():
+    msg = (
+        r"The 'DicomDir' class is deprecated and will be removed in v3.0, "
+        r"after which 'dcmread\(\)' will return a normal 'FileDataset' "
+        r"instance for 'Media Storage Directory' SOP Instances."
+    )
+    with pytest.warns(DeprecationWarning, match=msg):
+        ds = dcmread(TEST_FILE)
diff --git a/pydicom/tests/test_filereader.py b/pydicom/tests/test_filereader.py
index 091811ecc..c695a2862 100644
--- a/pydicom/tests/test_filereader.py
+++ b/pydicom/tests/test_filereader.py
@@ -19,7 +19,7 @@ from pydicom import config
 from pydicom.dataset import Dataset, FileDataset, FileMetaDataset
 from pydicom.data import get_testdata_file
 from pydicom.datadict import add_dict_entries
-from pydicom.filereader import dcmread, read_dataset
+from pydicom.filereader import dcmread, read_dataset, read_dicomdir
 from pydicom.dataelem import DataElement, DataElement_from_raw
 from pydicom.errors import InvalidDicomError
 from pydicom.filebase import DicomBytesIO
@@ -1566,3 +1566,13 @@ class TestDataElementGenerator:
         gen = data_element_generator(fp, False, False)
         elem = DataElement(0x00100010, "PN", "ABCDEF")
         assert elem == DataElement_from_raw(next(gen), "ISO_IR 100")
+
+
+def test_read_dicomdir_deprecated():
+    """Test deprecation warning for read_dicomdir()."""
+    msg = (
+        r"'read_dicomdir\(\)' is deprecated and will be removed in v3.0, use "
+        r"'dcmread\(\)' instead"
+    )
+    with pytest.warns(DeprecationWarning, match=msg):
+        ds = read_dicomdir(get_testdata_file("DICOMDIR"))
diff --git a/pydicom/tests/test_fileset.py b/pydicom/tests/test_fileset.py
index 079cb8c9c..a32411828 100644
--- a/pydicom/tests/test_fileset.py
+++ b/pydicom/tests/test_fileset.py
@@ -36,6 +36,7 @@ from pydicom.uid import (
     ExplicitVRLittleEndian, generate_uid, ImplicitVRLittleEndian
 )
 
+
 TEST_FILE = get_testdata_file('DICOMDIR')
 TINY_ALPHA_FILESET = get_testdata_file("tiny_alpha/DICOMDIR")
 IMPLICIT_TEST_FILE = get_testdata_file('DICOMDIR-implicit')
@@ -440,6 +441,7 @@ class TestGenerateFilename:
             next(generate_filename('A' * 8))
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestRecordNode:
     """Tests for RecordNode."""
     def test_root(self, private):
@@ -720,6 +722,7 @@ class TestRecordNode:
             instance.node._file_id
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestFileInstance:
     """Tests for FileInstance."""
     def test_getattr(self, dicomdir):
@@ -1015,6 +1018,7 @@ class TestFileInstance:
         assert ExplicitVRLittleEndian == instance.TransferSyntaxUID
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestFileSet:
     """Tests for FileSet."""
     def test_empty(self):
@@ -1841,6 +1845,7 @@ class TestFileSet:
         ) in str(fs)
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestFileSet_Load:
     """Tests for a loaded File-set."""
     def test_write_dicomdir(self, dicomdir):
@@ -2133,6 +2138,7 @@ class TestFileSet_Load:
         assert fs.UID == dicomdir.file_meta.MediaStorageSOPInstanceUID
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestFileSet_Modify:
     """Tests for a modified File-set."""
     def setup(self):
@@ -2436,6 +2442,7 @@ class TestFileSet_Modify:
             fs.add(ds)
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 class TestFileSet_Copy:
     """Tests for copying a File-set."""
     def setup(self):
@@ -2689,6 +2696,7 @@ REFERENCE_4LEVEL = [
 ]
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 @pytest.mark.parametrize("rtype, sop", REFERENCE_1LEVEL)
 def test_one_level_record(rtype, sop, dummy, tdir):
     """Test adding instances that require a single level hierarchy."""
@@ -2717,6 +2725,7 @@ def test_one_level_record(rtype, sop, dummy, tdir):
     fs.add(ds)
 
 
+@pytest.mark.filterwarnings("ignore:The 'DicomDir'")
 @pytest.mark.parametrize("rtype, sop, modality, kw", REFERENCE_4LEVEL)
 def test_four_level_record(rtype, sop, modality, kw, dummy, tdir):
     """Test adding instances that require the 4-level hierarchy."""