Merge lp:~jelmer/bzr/split-subsegment into lp:bzr

[Jelmer Vernooij]

This adds {split,join}_segment_parameters to urlutils - plumbing for colocated branches.

This adds bzrlib.urlutils.{join,split}_subsegments, both of which are nececessary for colocated branches.

[Robert Collins (lifeless)]

I think you would be better calling this 'segment parameters' or something
like that, and it should take a dict rather than plain strings, I think.

[Robert Collins (lifeless)]

Voting

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

On Mon, 2010-04-19 at 00:54 +0000, Robert Collins wrote:
> I think you would be better calling this 'segment parameters' or something
> like that, and it should take a dict rather than plain strings, I think.
Thanks for the review.

I'm not I understand having a dict rather than a list. What would you
want the dict to use as keys or values?

Cheers,

Jelmer

[Robert Collins (lifeless)]

We're adding parameters to a url; see the urllib API for doing that for
existing API's. it also got rehashed on python-dev just recently.

Basically, the API you're proposing has users do the parameter serialisation
themselves, its ugly ;)

[Jelmer Vernooij (jelmer)]

Updated, added a separate function that does the parameter handling.

[Robert Collins (lifeless)]

Further - and final.

Please test:
",foo=bar" etc
"foo/,foo=bar" etc
"foo/base,foo=bar/other/elements" etc

Please make the shortest functions the ones taking and returning dicts; and the others to be _raw or something; with a note in the docstring that they are lower level functions.

review: Needs Fixing

[lp:~jelmer/bzr/split-subsegment]

5168. By Jelmer Vernooij on 2010-05-14

rename {split,join}_subsegments -> {split,join}_segment_parameters_raw and add more tests.

[Robert Collins (lifeless)]

I know I said that that was my final request - I was wrong. Could you please:
use better examples - e.g.

rather than 'branch':'brrr' if you use 'key1':'value1', it will be a bit more obvious to people reading the tests what you expect to happen.

Secondly, in the docstrings for the methods, please make sure that someone reading just the docstring can figure out what the method will do and what they should pass in. E.g. rather than saying 'Dictionary of parameters' say 'Dictionary representing the url segment parameters. Each key:value in the dict is serialised as key=value. Keys and values must both be bytestrings.

And that brings up the hopefully last point - please typecheck that either each key,value is a bytestring, or that str(key) and str(value) is a bytestring [depending on whether you want to permit passing in objects with a __str__, or only actual strings.

Cheers,
Rob

review: Needs Fixing

[lp:~jelmer/bzr/split-subsegment]

5169. By Jelmer Vernooij on 2010-05-27

Fix example names in tests.

5170. By Jelmer Vernooij on 2010-05-27

Add type checking.

=== modified file 'bzrlib/tests/test_urlutils.py'
--- bzrlib/tests/test_urlutils.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_urlutils.py	2010-05-27 22:12:33 +0000
@@ -270,6 +270,51 @@
         self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '..')
         self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '/..')
 
+    def test_join_segment_parameters_raw(self):
+        join_segment_parameters_raw = urlutils.join_segment_parameters_raw
+        self.assertEquals("/somedir/path", 
+            join_segment_parameters_raw("/somedir/path"))
+        self.assertEquals("/somedir/path,rawdata", 
+            join_segment_parameters_raw("/somedir/path", "rawdata"))
+        self.assertRaises(InvalidURLJoin,
+            join_segment_parameters_raw, "/somedir/path",
+                "rawdata1,rawdata2,rawdata3")
+        self.assertEquals("/somedir/path,bla,bar",
+            join_segment_parameters_raw("/somedir/path", "bla", "bar"))
+        self.assertEquals("/somedir,exist=some/path,bla,bar",
+            join_segment_parameters_raw("/somedir,exist=some/path",
+                "bla", "bar"))
+        self.assertRaises(TypeError, join_segment_parameters_raw, 
+            "/somepath", 42)
+
+    def test_join_segment_parameters(self):
+        join_segment_parameters = urlutils.join_segment_parameters
+        self.assertEquals("/somedir/path", 
+            join_segment_parameters("/somedir/path", {}))
+        self.assertEquals("/somedir/path,key1=val1", 
+            join_segment_parameters("/somedir/path", {"key1": "val1"}))
+        self.assertRaises(InvalidURLJoin,
+            join_segment_parameters, "/somedir/path",
+            {"branch": "brr,brr,brr"})
+        self.assertRaises(InvalidURLJoin,
+            join_segment_parameters, "/somedir/path", {"key1=val1": "val2"})
+        self.assertEquals("/somedir/path,key1=val1,key2=val2",
+            join_segment_parameters("/somedir/path", {
+                "key1": "val1", "key2": "val2"}))
+        self.assertEquals("/somedir/path,key1=val1,key2=val2",
+            join_segment_parameters("/somedir/path,key1=val1", {
+                "key2": "val2"}))
+        self.assertEquals("/somedir/path,key1=val2",
+            join_segment_parameters("/somedir/path,key1=val1", {
+                "key1": "val2"}))
+        self.assertEquals("/somedir,exist=some/path,key1=val1",
+            join_segment_parameters("/somedir,exist=some/path",
+                {"key1": "val1"}))
+        self.assertEquals("/,key1=val1,key2=val2",
+            join_segment_parameters("/,key1=val1", {"key2": "val2"}))
+        self.assertRaises(TypeError,
+            join_segment_parameters, "/,key1=val1", {"foo": 42})
+
     def test_function_type(self):
         if sys.platform == 'win32':
             self.assertEqual(urlutils._win32_local_path_to_url, urlutils.local_path_to_url)
@@ -412,6 +457,53 @@
         self.assertEqual(('path/..', 'foo'), split('path/../foo'))
         self.assertEqual(('../path', 'foo'), split('../path/foo'))
 
+    def test_split_segment_parameters_raw(self):
+        split_segment_parameters_raw = urlutils.split_segment_parameters_raw
+        self.assertEquals(("/some/path", []),
+            split_segment_parameters_raw("/some/path"))
+        self.assertEquals(("/some/path", ["tip"]),
+            split_segment_parameters_raw("/some/path,tip"))
+        self.assertEquals(("/some,dir/path", ["tip"]),
+            split_segment_parameters_raw("/some,dir/path,tip"))
+        self.assertEquals(("/somedir/path", ["heads%2Ftip"]),
+            split_segment_parameters_raw("/somedir/path,heads%2Ftip"))
+        self.assertEquals(("/somedir/path", ["heads%2Ftip", "bar"]),
+            split_segment_parameters_raw("/somedir/path,heads%2Ftip,bar"))
+        self.assertEquals(("/", ["key1=val1"]),
+            split_segment_parameters_raw(",key1=val1"))
+        self.assertEquals(("foo/", ["key1=val1"]),
+            split_segment_parameters_raw("foo/,key1=val1"))
+        self.assertEquals(("foo/base,la=bla/other/elements", []),
+            split_segment_parameters_raw("foo/base,la=bla/other/elements"))
+        self.assertEquals(("foo/base,la=bla/other/elements", ["a=b"]),
+            split_segment_parameters_raw("foo/base,la=bla/other/elements,a=b"))
+
+    def test_split_segment_parameters(self):
+        split_segment_parameters = urlutils.split_segment_parameters
+        self.assertEquals(("/some/path", {}),
+            split_segment_parameters("/some/path"))
+        self.assertEquals(("/some/path", {"branch": "tip"}),
+            split_segment_parameters("/some/path,branch=tip"))
+        self.assertEquals(("/some,dir/path", {"branch": "tip"}),
+            split_segment_parameters("/some,dir/path,branch=tip"))
+        self.assertEquals(("/somedir/path", {"ref": "heads%2Ftip"}),
+            split_segment_parameters("/somedir/path,ref=heads%2Ftip"))
+        self.assertEquals(("/somedir/path",
+            {"ref": "heads%2Ftip", "key1": "val1"}),
+            split_segment_parameters(
+                "/somedir/path,ref=heads%2Ftip,key1=val1"))
+        self.assertEquals(("/somedir/path", {"ref": "heads%2F=tip"}),
+            split_segment_parameters("/somedir/path,ref=heads%2F=tip"))
+        self.assertEquals(("/", {"key1": "val1"}),
+            split_segment_parameters(",key1=val1"))
+        self.assertEquals(("foo/", {"key1": "val1"}),
+            split_segment_parameters("foo/,key1=val1"))
+        self.assertEquals(("foo/base,key1=val1/other/elements", {}),
+            split_segment_parameters("foo/base,key1=val1/other/elements"))
+        self.assertEquals(("foo/base,key1=val1/other/elements",
+            {"key2": "val2"}), split_segment_parameters(
+                "foo/base,key1=val1/other/elements,key2=val2"))
+
     def test_win32_strip_local_trailing_slash(self):
         strip = urlutils._win32_strip_local_trailing_slash
         self.assertEqual('file://', strip('file://'))

=== modified file 'bzrlib/urlutils.py'
--- bzrlib/urlutils.py	2010-02-17 17:11:16 +0000
+++ bzrlib/urlutils.py	2010-05-27 22:12:33 +0000
@@ -469,6 +469,78 @@
     return url_base + head, tail
 
 
+def split_segment_parameters_raw(url):
+    """Split the subsegment of the last segment of a URL.
+
+    :param url: A relative or absolute URL
+    :return: (url, subsegments)
+    """
+    (parent_url, child_dir) = split(url)
+    subsegments = child_dir.split(",")
+    if len(subsegments) == 1:
+        return (url, [])
+    return (join(parent_url, subsegments[0]), subsegments[1:])
+
+
+def split_segment_parameters(url):
+    """Split the segment parameters of the last segment of a URL.
+
+    :param url: A relative or absolute URL
+    :return: (url, segment_parameters)
+    """
+    (base_url, subsegments) = split_segment_parameters_raw(url)
+    parameters = {}
+    for subsegment in subsegments:
+        (key, value) = subsegment.split("=", 1)
+        parameters[key] = value
+    return (base_url, parameters)
+
+
+def join_segment_parameters_raw(base, *subsegments):
+    """Create a new URL by adding subsegments to an existing one. 
+
+    This adds the specified subsegments to the last path in the specified
+    base URL. The subsegments should be bytestrings.
+
+    :note: You probably want to use join_segment_parameters instead.
+    """
+    if not subsegments:
+        return base
+    for subsegment in subsegments:
+        if type(subsegment) is not str:
+            raise TypeError("Subsegment %r is not a bytestring" % subsegment)
+        if "," in subsegment:
+            raise errors.InvalidURLJoin(", exists in subsegments",
+                                        base, subsegments)
+    return ",".join((base,) + subsegments)
+
+
+def join_segment_parameters(url, parameters):
+    """Create a new URL by adding segment parameters to an existing one.
+
+    The parameters of the last segment in the URL will be updated; if a
+    parameter with the same key already exists it will be overwritten.
+
+    :param url: A URL, as string
+    :param parameters: Dictionary of parameters, keys and values as bytestrings
+    """
+    (base, existing_parameters) = split_segment_parameters(url)
+    new_parameters = {}
+    new_parameters.update(existing_parameters)
+    for key, value in parameters.iteritems():
+        if type(key) is not str:
+            raise TypeError("parameter key %r is not a bytestring" % key)
+        if type(value) is not str:
+            raise TypeError("parameter value %r for %s is not a bytestring" %
+                (key, value))
+        if "=" in key:
+            raise errors.InvalidURLJoin("= exists in parameter key", url,
+                parameters)
+        new_parameters[key] = value
+    return join_segment_parameters_raw(base, 
+        *["%s=%s" % item for item in sorted(new_parameters.items())])
+
+
 def _win32_strip_local_trailing_slash(url):
     """Strip slashes after the drive letter"""
     if len(url) > WIN32_MIN_ABS_FILEURL_LENGTH: