Bazaar

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

  1. split-subsegment
  2. Merge into bzr.dev
Proposed by Jelmer Vernooij
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 5267
Proposed branch: lp:~jelmer/bzr/split-subsegment
Merge into: lp:bzr
Diff against target: 192 lines (+164/-0)
2 files modified
bzrlib/tests/test_urlutils.py (+92/-0)
bzrlib/urlutils.py (+72/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/split-subsegment
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
Bazaar Developers code Pending
Review via email: mp+23611@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) 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.

Revision history for this message
Robert Collins (lifeless) wrote :

Voting

review: Needs Fixing
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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 ;)

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/test_urlutils.py'
2--- bzrlib/tests/test_urlutils.py 2010-02-17 17:11:16 +0000
3+++ bzrlib/tests/test_urlutils.py 2010-05-27 22:12:33 +0000
4@@ -270,6 +270,51 @@
5 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '..')
6 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '/..')
7
8+ def test_join_segment_parameters_raw(self):
9+ join_segment_parameters_raw = urlutils.join_segment_parameters_raw
10+ self.assertEquals("/somedir/path",
11+ join_segment_parameters_raw("/somedir/path"))
12+ self.assertEquals("/somedir/path,rawdata",
13+ join_segment_parameters_raw("/somedir/path", "rawdata"))
14+ self.assertRaises(InvalidURLJoin,
15+ join_segment_parameters_raw, "/somedir/path",
16+ "rawdata1,rawdata2,rawdata3")
17+ self.assertEquals("/somedir/path,bla,bar",
18+ join_segment_parameters_raw("/somedir/path", "bla", "bar"))
19+ self.assertEquals("/somedir,exist=some/path,bla,bar",
20+ join_segment_parameters_raw("/somedir,exist=some/path",
21+ "bla", "bar"))
22+ self.assertRaises(TypeError, join_segment_parameters_raw,
23+ "/somepath", 42)
24+
25+ def test_join_segment_parameters(self):
26+ join_segment_parameters = urlutils.join_segment_parameters
27+ self.assertEquals("/somedir/path",
28+ join_segment_parameters("/somedir/path", {}))
29+ self.assertEquals("/somedir/path,key1=val1",
30+ join_segment_parameters("/somedir/path", {"key1": "val1"}))
31+ self.assertRaises(InvalidURLJoin,
32+ join_segment_parameters, "/somedir/path",
33+ {"branch": "brr,brr,brr"})
34+ self.assertRaises(InvalidURLJoin,
35+ join_segment_parameters, "/somedir/path", {"key1=val1": "val2"})
36+ self.assertEquals("/somedir/path,key1=val1,key2=val2",
37+ join_segment_parameters("/somedir/path", {
38+ "key1": "val1", "key2": "val2"}))
39+ self.assertEquals("/somedir/path,key1=val1,key2=val2",
40+ join_segment_parameters("/somedir/path,key1=val1", {
41+ "key2": "val2"}))
42+ self.assertEquals("/somedir/path,key1=val2",
43+ join_segment_parameters("/somedir/path,key1=val1", {
44+ "key1": "val2"}))
45+ self.assertEquals("/somedir,exist=some/path,key1=val1",
46+ join_segment_parameters("/somedir,exist=some/path",
47+ {"key1": "val1"}))
48+ self.assertEquals("/,key1=val1,key2=val2",
49+ join_segment_parameters("/,key1=val1", {"key2": "val2"}))
50+ self.assertRaises(TypeError,
51+ join_segment_parameters, "/,key1=val1", {"foo": 42})
52+
53 def test_function_type(self):
54 if sys.platform == 'win32':
55 self.assertEqual(urlutils._win32_local_path_to_url, urlutils.local_path_to_url)
56@@ -412,6 +457,53 @@
57 self.assertEqual(('path/..', 'foo'), split('path/../foo'))
58 self.assertEqual(('../path', 'foo'), split('../path/foo'))
59
60+ def test_split_segment_parameters_raw(self):
61+ split_segment_parameters_raw = urlutils.split_segment_parameters_raw
62+ self.assertEquals(("/some/path", []),
63+ split_segment_parameters_raw("/some/path"))
64+ self.assertEquals(("/some/path", ["tip"]),
65+ split_segment_parameters_raw("/some/path,tip"))
66+ self.assertEquals(("/some,dir/path", ["tip"]),
67+ split_segment_parameters_raw("/some,dir/path,tip"))
68+ self.assertEquals(("/somedir/path", ["heads%2Ftip"]),
69+ split_segment_parameters_raw("/somedir/path,heads%2Ftip"))
70+ self.assertEquals(("/somedir/path", ["heads%2Ftip", "bar"]),
71+ split_segment_parameters_raw("/somedir/path,heads%2Ftip,bar"))
72+ self.assertEquals(("/", ["key1=val1"]),
73+ split_segment_parameters_raw(",key1=val1"))
74+ self.assertEquals(("foo/", ["key1=val1"]),
75+ split_segment_parameters_raw("foo/,key1=val1"))
76+ self.assertEquals(("foo/base,la=bla/other/elements", []),
77+ split_segment_parameters_raw("foo/base,la=bla/other/elements"))
78+ self.assertEquals(("foo/base,la=bla/other/elements", ["a=b"]),
79+ split_segment_parameters_raw("foo/base,la=bla/other/elements,a=b"))
80+
81+ def test_split_segment_parameters(self):
82+ split_segment_parameters = urlutils.split_segment_parameters
83+ self.assertEquals(("/some/path", {}),
84+ split_segment_parameters("/some/path"))
85+ self.assertEquals(("/some/path", {"branch": "tip"}),
86+ split_segment_parameters("/some/path,branch=tip"))
87+ self.assertEquals(("/some,dir/path", {"branch": "tip"}),
88+ split_segment_parameters("/some,dir/path,branch=tip"))
89+ self.assertEquals(("/somedir/path", {"ref": "heads%2Ftip"}),
90+ split_segment_parameters("/somedir/path,ref=heads%2Ftip"))
91+ self.assertEquals(("/somedir/path",
92+ {"ref": "heads%2Ftip", "key1": "val1"}),
93+ split_segment_parameters(
94+ "/somedir/path,ref=heads%2Ftip,key1=val1"))
95+ self.assertEquals(("/somedir/path", {"ref": "heads%2F=tip"}),
96+ split_segment_parameters("/somedir/path,ref=heads%2F=tip"))
97+ self.assertEquals(("/", {"key1": "val1"}),
98+ split_segment_parameters(",key1=val1"))
99+ self.assertEquals(("foo/", {"key1": "val1"}),
100+ split_segment_parameters("foo/,key1=val1"))
101+ self.assertEquals(("foo/base,key1=val1/other/elements", {}),
102+ split_segment_parameters("foo/base,key1=val1/other/elements"))
103+ self.assertEquals(("foo/base,key1=val1/other/elements",
104+ {"key2": "val2"}), split_segment_parameters(
105+ "foo/base,key1=val1/other/elements,key2=val2"))
106+
107 def test_win32_strip_local_trailing_slash(self):
108 strip = urlutils._win32_strip_local_trailing_slash
109 self.assertEqual('file://', strip('file://'))
110
111=== modified file 'bzrlib/urlutils.py'
112--- bzrlib/urlutils.py 2010-02-17 17:11:16 +0000
113+++ bzrlib/urlutils.py 2010-05-27 22:12:33 +0000
114@@ -469,6 +469,78 @@
115 return url_base + head, tail
116
117
118+def split_segment_parameters_raw(url):
119+ """Split the subsegment of the last segment of a URL.
120+
121+ :param url: A relative or absolute URL
122+ :return: (url, subsegments)
123+ """
124+ (parent_url, child_dir) = split(url)
125+ subsegments = child_dir.split(",")
126+ if len(subsegments) == 1:
127+ return (url, [])
128+ return (join(parent_url, subsegments[0]), subsegments[1:])
129+
130+
131+def split_segment_parameters(url):
132+ """Split the segment parameters of the last segment of a URL.
133+
134+ :param url: A relative or absolute URL
135+ :return: (url, segment_parameters)
136+ """
137+ (base_url, subsegments) = split_segment_parameters_raw(url)
138+ parameters = {}
139+ for subsegment in subsegments:
140+ (key, value) = subsegment.split("=", 1)
141+ parameters[key] = value
142+ return (base_url, parameters)
143+
144+
145+def join_segment_parameters_raw(base, *subsegments):
146+ """Create a new URL by adding subsegments to an existing one.
147+
148+ This adds the specified subsegments to the last path in the specified
149+ base URL. The subsegments should be bytestrings.
150+
151+ :note: You probably want to use join_segment_parameters instead.
152+ """
153+ if not subsegments:
154+ return base
155+ for subsegment in subsegments:
156+ if type(subsegment) is not str:
157+ raise TypeError("Subsegment %r is not a bytestring" % subsegment)
158+ if "," in subsegment:
159+ raise errors.InvalidURLJoin(", exists in subsegments",
160+ base, subsegments)
161+ return ",".join((base,) + subsegments)
162+
163+
164+def join_segment_parameters(url, parameters):
165+ """Create a new URL by adding segment parameters to an existing one.
166+
167+ The parameters of the last segment in the URL will be updated; if a
168+ parameter with the same key already exists it will be overwritten.
169+
170+ :param url: A URL, as string
171+ :param parameters: Dictionary of parameters, keys and values as bytestrings
172+ """
173+ (base, existing_parameters) = split_segment_parameters(url)
174+ new_parameters = {}
175+ new_parameters.update(existing_parameters)
176+ for key, value in parameters.iteritems():
177+ if type(key) is not str:
178+ raise TypeError("parameter key %r is not a bytestring" % key)
179+ if type(value) is not str:
180+ raise TypeError("parameter value %r for %s is not a bytestring" %
181+ (key, value))
182+ if "=" in key:
183+ raise errors.InvalidURLJoin("= exists in parameter key", url,
184+ parameters)
185+ new_parameters[key] = value
186+ return join_segment_parameters_raw(base,
187+ *["%s=%s" % item for item in sorted(new_parameters.items())])
188+
189+
190 def _win32_strip_local_trailing_slash(url):
191 """Strip slashes after the drive letter"""
192 if len(url) > WIN32_MIN_ABS_FILEURL_LENGTH: