deprecate preserve_comments

this argument appears to have no effect other
than breaking specs with comments that contain
a closing parenthesis

src/stpipe/cmdline.py

@@ -279,7 +279,7 @@ def just_the_step_from_cmdline(args, cls=None):
     # all of the expected reference files
 
     # load_spec_file is a method of both Step and Pipeline
-    spec = step_class.load_spec_file(preserve_comments=True)
+    spec = step_class.load_spec_file()
 
     parser2 = _build_arg_parser_from_spec(spec, step_class, parent=parser1)
 

src/stpipe/config_parser.py

@@ -5,6 +5,7 @@
 import os
 import os.path
 import textwrap
+import warnings
 from inspect import isclass
 
 from asdf import ValidationError as AsdfValidationError
@@ -20,6 +21,7 @@
     get_extra_values,
 )
 from .extern.configobj.validate import ValidateError, Validator, VdtTypeError
+from .utilities import _not_set
 
 # Configure logger
 logger = logging.getLogger(__name__)
@@ -130,7 +132,7 @@ def _config_obj_from_step_config(config):
     return configobj
 
 
-def get_merged_spec_file(cls, preserve_comments=False):
+def get_merged_spec_file(cls, preserve_comments=_not_set):
     """
     Creates a merged spec file for a Step class and all of its
     subclasses.
@@ -141,6 +143,8 @@ def get_merged_spec_file(cls, preserve_comments=False):
         A class or instance of a `Step`-based class.
 
     preserve_comments : bool, optional
+        This argument is deprecated and appears to have no
+        effect on the returned spec.
         When True, preserve the comments in the spec file
     """
     if not isclass(cls):
@@ -162,7 +166,7 @@ def get_merged_spec_file(cls, preserve_comments=False):
     return config
 
 
-def load_spec_file(cls, preserve_comments=False):
+def load_spec_file(cls, preserve_comments=_not_set):
     """
     Load the spec file corresponding to the given class.
 
@@ -172,13 +176,18 @@ def load_spec_file(cls, preserve_comments=False):
         A class or instance of a `Step`-based class.
 
     preserve_comments: bool
+        This argument is deprecated and appears to have no
+        effect on the returned spec.
         True to keep comments in the resulting `ConfigObj`
 
     Returns
     -------
     spec_file: ConfigObj
         The resulting configuration object
     """
+    if preserve_comments is not _not_set:
+        msg = "preserve_comments is deprecated"
+        warnings.warn(msg, DeprecationWarning)
     # Don't use 'hasattr' here, because we don't want to inherit spec
     # from the base class.
     if not isclass(cls):

src/stpipe/pipeline.py

@@ -7,6 +7,7 @@
 from . import config_parser, crds_client, log
 from .extern.configobj.configobj import ConfigObj, Section
 from .step import Step, get_disable_crds_steppars
+from .utilities import _not_set
 
 # For classmethods, the logger to use is the
 # delegator, since the pipeline has not yet been instantiated.
@@ -112,7 +113,7 @@ def merge_config(cls, config, config_file):
         return config
 
     @classmethod
-    def load_spec_file(cls, preserve_comments=False):
+    def load_spec_file(cls, preserve_comments=_not_set):
         spec = config_parser.get_merged_spec_file(
             cls, preserve_comments=preserve_comments
         )

src/stpipe/step.py

@@ -29,6 +29,7 @@
 from . import config, config_parser, crds_client, log, utilities
 from .datamodel import AbstractDataModel
 from .format_template import FormatTemplate
+from .utilities import _not_set
 
 
 class Step:
@@ -90,7 +91,7 @@ def merge_config(cls, config, config_file):
         return config
 
     @classmethod
-    def load_spec_file(cls, preserve_comments=False):
+    def load_spec_file(cls, preserve_comments=_not_set):
         spec = config_parser.get_merged_spec_file(
             cls, preserve_comments=preserve_comments
         )
@@ -105,7 +106,7 @@ def load_spec_file(cls, preserve_comments=False):
 
     @classmethod
     def print_configspec(cls):
-        specfile = cls.load_spec_file(preserve_comments=True)
+        specfile = cls.load_spec_file()
         specfile.write(sys.stdout.buffer)
 
     @classmethod

src/stpipe/utilities.py

@@ -135,3 +135,16 @@ def get_fully_qualified_class_name(cls_or_obj):
         return cls.__name__  # Avoid reporting __builtin__
     else:
         return module + "." + cls.__name__
+
+
+class _NotSet:
+    """
+    Special value indicating that a parameter is not set.  Distinct
+    from None. Instead of using this class use the _not_set instance
+    below
+    """
+
+    pass
+
+
+_not_set = _NotSet()

tests/test_config_parser.py

@@ -1,7 +1,11 @@
+import contextlib
 from collections.abc import Mapping
 
+import pytest
+
 from stpipe import config_parser
 from stpipe.extern.configobj.configobj import ConfigObj, Section
+from stpipe.utilities import _not_set
 
 
 def test_merge_config_nested_mapping():
@@ -31,3 +35,26 @@ def __len__(self):
     config_parser.merge_config(config, {"foo": TestMapping({"bar": "baz"})})
     assert isinstance(config["foo"], TestMapping)
     assert config["foo"]["bar"] == "baz"
+
+
+@pytest.mark.parametrize("value", [True, False, None, _not_set])
+def test_preserve_comments_deprecation(value):
+    class Foo:
+        spec = """
+        # initial comment
+        bar = string(default='bam')  # an inline comment (with parentheses)
+        # final comment
+        """
+
+    # if preserve_comments is _not_set there should be no warning
+    if value is _not_set:
+        ctx = contextlib.nullcontext()
+    else:
+        ctx = pytest.warns(DeprecationWarning)
+
+    with ctx:
+        spec = config_parser.load_spec_file(Foo, preserve_comments=value)
+
+    assert "initial comment" in spec.initial_comment[0]
+    assert "final comment" in spec.final_comment[0]
+    assert "inline comment (with parentheses)" in spec.inline_comments["bar"]