Repository: planetarypy/pvl

Issue:
We probably need a variant PDS3LabelEncoder().
The various PVL "flavors" sometimes have asymmetric rules for "reading" and "writing."  They are generally more permissive on "read" and strict on "write."

For example, for PDS3 Labels:

PDS3 Reads | `pvl` converts to Python | PDS3 Writes
-------------|-------------------------|-------------
2000-02-25T10:54:12.129Z | `datetime` | 2000-02-25T10:54:12.129Z
2000-02-25T10:54:12.129  | `datetime` | 2000-02-25T10:54:12.129Z
'I am a Symbol String' | `str` | 'I am a Symbol String'
"I'm a Text String" | `str` | "I'm a Text String"
"I am double-quoted" | `str` | 'I am double-quoted'

See the asymmetry?  

PDS3 Label "readers" can read times without any kind of timezone specifier, or with the "Z" specifier, but PDS3 Label "writers" must write out the trailing "Z" (see #82 for too much information).  

Similarly, did you know that ODL/PDS3 considers single-quoted strings as 'Symbol Strings' with restrictions on what can be inside them, and double-quoted "Text Strings" which can contain anything.  So on "write" our `pvl.encoder.PDS3LabelEncoder` sees if a Python `str` qualifies as a 'Symbol String' and writes it out with single quotes, and as a double-quoted "Text String" otherwise.

If a user wanted to get some PVL-text that could be read by a PDS3 "reader" but wanted no trailing "Z" or some text that qualified as a Symbol String to be double-quoted, we don't have a good mechansim for that, beyond them extending our `PDS3LabelEncoder()`.

**Describe the solution you'd like**
I think we should keep the `pvl.encoder.PDS3LabelEncoder()` as it is, meaning that it is very strict.  However, we should create a `pvl.encoder.PDS3VariantEncoder()` that allows users to specify which of the "optional" formatting they'd like to allow, but still write out PVL-text that is "readable" by our `pvl.decoder.PDS3Decoder()`.

**Describe alternatives you've considered**
I thought about simply putting optional parameters in the existing `pvl.encoder.PDS3LabelEncoder()` that would modify the behavior, however, while users may have an option, the ODL/PDS3 Spec on writing isn't really optional.  So I'd rather keep the `pvl.encoder.PDS3LabelEncoder()` very strict and immutable, but still provide this other encoder.

This also made me think more about allowing more "smart" types that we decode PVL-text to that "remember" the PVL-text that they came from (as Michael suggested in #81).  If the library could determine whether there was or wasn't a "Z" on the text that became a `datetime` then it could write it back out the same way.  Similarly with the different string quoting patterns.  That's all still valid, but this question of what to "allow" on output would still remain.

**Additional context**
The context here is that there are many bits of software out there that "read" PVL-text, and they do not all fully implement the PDS3 "reader" spec.  So someone could properly use the PDS3 spec to create a time with a trailing "Z", which would validate and be archived with the PDS, but software that "thinks" it works on PDS3 PVL-text might barf on the "Z" because it isn't properly implementing the "reading" of PDS3 labels.  

However, if someone didn't properly use the PDS3 spec, and wrote times without the trailing "Z", those labels **would still validate and be archived**, because the validators are "readers" that can read both (stupid asymmetric standard).  In practice, this has happened quite a bit, and if a developer was making some software by just looking at a corpus of PVL-text (and not closely reading the ODL/PDS3 spec), that software may not handle the "Z".

As always, there's the spec, and then there's how people use things in practice.  Having this variant encoder would help us support a wider variety of the ways that people are practically using PDS3 PVL-text, while still being "readable" by official PDS3 "readers."

Discussion:

Patch:
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index 5312514..f13d83c 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -62,8 +62,12 @@ Ready to contribute? Here's how to set up `pvl` for local development.
    If you are a conda user::
 
     $ cd pvl/
-    $ conda env create -n pvl -f environment.yml
+    $ conda env create -n pvldev -f environment.yml
+    $ conda activate pvldev
+    $ pip install --no-deps -e .
 
+   The last `pip install` installs pvl in "editable" mode which facilitates
+   testing.
 
 4. Create a branch for local 
    development::
diff --git a/README.rst b/README.rst
index cd56190..3710f1c 100644
--- a/README.rst
+++ b/README.rst
@@ -35,7 +35,7 @@ Python implementation of a PVL (Parameter Value Language) library.
 * `PlanetaryPy`_ Affiliate Package.
 
 PVL is a markup language, similar to XML, commonly employed for
-entries in the Planetary Database System used by NASA to store
+entries in the Planetary Data System used by NASA to archive
 mission data, among other uses.  This package supports both encoding
 and decoding a variety of PVL 'flavors' including PVL itself, ODL,
 `NASA PDS 3 Labels`_, and `USGS ISIS Cube Labels`_.
diff --git a/pvl/encoder.py b/pvl/encoder.py
index 8198fe5..4c8a916 100644
--- a/pvl/encoder.py
+++ b/pvl/encoder.py
@@ -825,20 +825,34 @@ class PDSLabelEncoder(ODLEncoder):
 
     You are not allowed to chose *end_delimiter* or *newline*
     as the parent class allows, because to be PDS-compliant,
-    those are fixed choices.
-
-    In PVL and ODL, the OBJECT and GROUP aggregations are
-    interchangable, but the PDS applies restrictions to what can
-    appear in a GROUP.  If *convert_group_to_object* is True,
-    and a GROUP does not conform to the PDS definition of a GROUP,
-    then it will be written out as an OBJECT.  If it is False,
-    then an exception will be thrown if incompatible GROUPs are
-    encountered.
-
-    *tab_replace* should indicate the number of space characters
-    to replace horizontal tab characters with (since tabs aren't
-    allowed in PDS labels).  If this is set to zero, tabs will not
-    be replaced with spaces.  Defaults to 4.
+    those are fixed choices.  However, in some cases, the PDS3
+    Standards are asymmetric, allowing for a wider variety of
+    PVL-text on "read" and a more narrow variety of PVL-text
+    on "write".  The default values of the PDSLabelEncoder enforce
+    those strict "write" rules, but if you wish to alter them,
+    but still produce PVL-text that would validate against the PDS3
+    standard, you may alter them.
+
+    :param convert_group_to_object: Defaults to True, meaning that
+        if a GROUP does not conform to the PDS definition of a
+        GROUP, then it will be written out as an OBJECT.  If it is
+        False, then an exception will be thrown if incompatible
+        GROUPs are encountered. In PVL and ODL, the OBJECT and GROUP
+        aggregations are interchangeable, but the PDS applies
+        restrictions to what can appear in a GROUP.
+    :param tab_replace: Defaults to 4 and indicates the number of
+        space characters to replace horizontal tab characters with
+        (since tabs aren't allowed in PDS labels).  If this is set
+        to zero, tabs will not be replaced with spaces.
+    :param symbol_single_quotes: Defaults to True, and if a Python `str`
+        object qualifies as a PVL Symbol String, it will be written to
+        PVL-text as a single-quoted string.  If False, no special
+        handling is done, and any PVL Symbol String will be treated
+        as a PVL Text String, which is typically enclosed with double-quotes.
+    :param time_trailing_z: defaults to True, and suffixes a "Z" to
+        datetimes and times written to PVL-text as the PDS encoding
+        standard requires.  If False, no trailing "Z" is written.
+
     """
 
     def __init__(
@@ -850,6 +864,8 @@ class PDSLabelEncoder(ODLEncoder):
         aggregation_end=True,
         convert_group_to_object=True,
         tab_replace=4,
+        symbol_single_quote=True,
+        time_trailing_z=True,
     ):
 
         if grammar is None:
@@ -870,6 +886,8 @@ class PDSLabelEncoder(ODLEncoder):
 
         self.convert_group_to_object = convert_group_to_object
         self.tab_replace = tab_replace
+        self.symbol_single_quote = symbol_single_quote
+        self.time_trailing_z = time_trailing_z
 
     def count_aggs(
         self, module: abc.Mapping, obj_count: int = 0, grp_count: int = 0
@@ -1042,6 +1060,18 @@ class PDSLabelEncoder(ODLEncoder):
 
         return super().encode_set(values)
 
+    def encode_string(self, value):
+        """Extends parent function to treat Symbol Strings as Text Strings,
+        which typically means that they are double-quoted and not
+        single-quoted.
+        """
+        if self.decoder.is_identifier(value):
+            return value
+        elif self.is_symbol(value) and self.symbol_single_quote:
+            return "'" + value + "'"
+        else:
+            return super(ODLEncoder, self).encode_string(value)
+
     def encode_time(self, value: datetime.time) -> str:
         """Overrides parent's encode_time() function because
         even though ODL allows for timezones, PDS does not.
@@ -1070,15 +1100,18 @@ class PDSLabelEncoder(ODLEncoder):
             s += f":{value:%S}"
 
         if (
-            value.tzinfo is not None and
-            value.utcoffset() != datetime.timedelta()
+            value.tzinfo is None or
+            value.tzinfo.utcoffset(None) == datetime.timedelta(0)
         ):
+            if self.time_trailing_z:
+                return s + "Z"
+            else:
+                return s
+        else:
             raise ValueError(
                 "PDS labels should only have UTC times, but "
                 f"this time has a timezone: {value}"
             )
-        else:
-            return s + "Z"
 
 
 class ISISEncoder(PVLEncoder):


Test patch:
diff --git a/tests/test_encoder.py b/tests/test_encoder.py
index 864d3e2..91669c0 100644
--- a/tests/test_encoder.py
+++ b/tests/test_encoder.py
@@ -283,6 +283,13 @@ END_OBJECT = key"""
         t = datetime.time(10, 54, 12, 123456, tzinfo=datetime.timezone.utc)
         self.assertRaises(ValueError, self.e.encode_time, t)
 
+        e = PDSLabelEncoder(time_trailing_z=False)
+        self.assertEqual("01:02", e.encode_time(datetime.time(1, 2)))
+
+    def test_encode_string(self):
+        e = PDSLabelEncoder(symbol_single_quote=False)
+        self.assertEqual('"AB CD"', e.encode_string('AB CD'))
+
     def test_encode(self):
         m = PVLModule(a=PVLGroup(g1=2, g2=3.4), b="c")