Bug 1333564 - [manifestparser] Add support for inline comments, r?jmaher draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Wed, 25 Jan 2017 12:21:31 -0500
changeset 466826 7eca443a73dff58c39dddc932a423fa8aa086b03
parent 466825 feff0c1c04f933b583b3641222177607eef5bee9
child 466827 d3856c1db2ecba3bda1cc91f269ea6e91c1231f6
push id43008
push userahalberstadt@mozilla.com
push dateThu, 26 Jan 2017 17:34:22 +0000
reviewersjmaher
bugs1333564
milestone54.0a1
Bug 1333564 - [manifestparser] Add support for inline comments, r?jmaher Previously manifestparser only supported inline comments on conditional keys, such as skip-if, fails-if, subsuite-if, etc. But on any other key name, inline comments weren't supported. This was a bad situation because people saw these skip-if comments, and naturally assumed inline comments worked everywhere. Then they were surprised when things broke in mysterious ways. This patch removes the special-casing for skip-if and friends and allows inline comments everywhere. A caveat to this, is that ' #' is no longer a valid substring in a value. MozReview-Commit-ID: Hr0BIwzTgaJ
testing/mozbase/docs/manifestparser.rst
testing/mozbase/manifestparser/manifestparser/ini.py
testing/mozbase/manifestparser/tests/test_read_ini.py
--- a/testing/mozbase/docs/manifestparser.rst
+++ b/testing/mozbase/docs/manifestparser.rst
@@ -131,27 +131,35 @@ section, without adding possible include
 
 .. code-block:: text
 
     [parent:../manifest.ini]
 
 Manifests are included relative to the directory of the manifest with
 the `[include:]` directive unless they are absolute paths.
 
-By default you can use '#' as a comment character. Comments must start on
-a new line, inline comments are not supported.
+By default you can use '#' as a comment character. Comments can start a
+new line, or be inline.
 
 .. code-block:: text
 
     [roses.js]
     # a valid comment
-    color = red # not a valid comment
+    color = red # another valid comment
+
+Comment characters must be preceded by a space, or they will not be
+treated as comments.
 
-In the example above, the 'color' property will have the value 'red #
-not a valid comment'.
+.. code-block:: text
+
+    [test1.js]
+    url = https://foo.com/bar#baz
+
+The '#baz' anchor will not be stripped off, as it wasn't preceded by
+a space.
 
 Special variable server-root
 ````````````````````````````
 There is a special variable called `server-root` used for paths on the system.
 This variable is deemed a path and will be expanded into its absolute form.
 
 Because of the inheritant nature of the key/value pairs, if one requires a
 system path, it must be absolute for it to be of any use in any included file.
--- a/testing/mozbase/manifestparser/manifestparser/ini.py
+++ b/testing/mozbase/manifestparser/manifestparser/ini.py
@@ -1,13 +1,14 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this file,
 # You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
+import sys
 
 __all__ = ['read_ini', 'combine_fields']
 
 
 def read_ini(fp, variables=None, default='DEFAULT', defaults_only=False,
              comments=None, separators=None, strict=True, handle_defaults=True):
     """
     read an .ini file and return a list of [(section, values)]
@@ -41,16 +42,33 @@ def read_ini(fp, variables=None, default
             # reset key and value to avoid continuation lines
             key = value = None
             continue
 
         # ignore comment lines
         if any(stripped.startswith(c) for c in comments):
             continue
 
+        # strip inline comments (borrowed from configparser)
+        comment_start = sys.maxsize
+        inline_prefixes = {p: -1 for p in comments}
+        while comment_start == sys.maxsize and inline_prefixes:
+            next_prefixes = {}
+            for prefix, index in inline_prefixes.items():
+                index = line.find(prefix, index+1)
+                if index == -1:
+                    continue
+                next_prefixes[prefix] = index
+                if index == 0 or (index > 0 and line[index-1].isspace()):
+                    comment_start = min(comment_start, index)
+            inline_prefixes = next_prefixes
+
+        if comment_start != sys.maxsize:
+            stripped = stripped[:comment_start].rstrip()
+
         # check for a new section
         if len(stripped) > 2 and stripped[0] == '[' and stripped[-1] == ']':
             section = stripped[1:-1].strip()
             key = value = None
 
             # deal with DEFAULT section
             if section.lower() == default.lower():
                 if strict:
@@ -133,11 +151,10 @@ def combine_fields(global_vars, local_va
     }
     final_mapping = global_vars.copy()
     for field_name, value in local_vars.items():
         if field_name not in field_patterns or field_name not in global_vars:
             final_mapping[field_name] = value
             continue
         global_value = global_vars[field_name]
         pattern = field_patterns[field_name]
-        final_mapping[field_name] = pattern % (
-            global_value.split('#')[0], value.split('#')[0])
+        final_mapping[field_name] = pattern % (global_value, value)
     return final_mapping
--- a/testing/mozbase/manifestparser/tests/test_read_ini.py
+++ b/testing/mozbase/manifestparser/tests/test_read_ini.py
@@ -7,45 +7,32 @@ ensure our .ini parser is doing what we 
 python's standard ConfigParser when 2.7 is reality so OrderedDict
 is the default:
 
 http://docs.python.org/2/library/configparser.html
 """
 
 import unittest
 from manifestparser import read_ini
-from ConfigParser import ConfigParser
 from StringIO import StringIO
 
 import mozunit
 
 
 class IniParserTest(unittest.TestCase):
 
     def test_inline_comments(self):
-        """
-        We have no inline comments; so we're testing to ensure we don't:
-        https://bugzilla.mozilla.org/show_bug.cgi?id=855288
-        """
-
-        # test '#' inline comments (really, the lack thereof)
-        string = """[test_felinicity.py]
+        manifest = """
+[test_felinicity.py]
 kittens = true # This test requires kittens
+cats = false#but not cats
 """
-        buffer = StringIO()
-        buffer.write(string)
-        buffer.seek(0)
-        result = read_ini(buffer)[0][1]['kittens']
-        self.assertEqual(result, "true # This test requires kittens")
-
-        # compare this to ConfigParser
-        # python 2.7 ConfigParser does not support '#' as an
-        # inline comment delimeter (for "backwards compatability"):
-        # http://docs.python.org/2/library/configparser.html
-        buffer.seek(0)
-        parser = ConfigParser()
-        parser.readfp(buffer)
-        control = parser.get('test_felinicity.py', 'kittens')
-        self.assertEqual(result, control)
+        # make sure inline comments get stripped out, but comments without a space in front don't
+        buf = StringIO()
+        buf.write(manifest)
+        buf.seek(0)
+        result = read_ini(buf)[0][1]
+        self.assertEqual(result['kittens'], 'true')
+        self.assertEqual(result['cats'], "false#but not cats")
 
 
 if __name__ == '__main__':
     mozunit.main()