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
=== 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 @@
270 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '..')270 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '..')
271 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '/..')271 self.assertRaises(InvalidURLJoin, urlutils.joinpath, '/', '/..')
272272
273 def test_join_segment_parameters_raw(self):
274 join_segment_parameters_raw = urlutils.join_segment_parameters_raw
275 self.assertEquals("/somedir/path",
276 join_segment_parameters_raw("/somedir/path"))
277 self.assertEquals("/somedir/path,rawdata",
278 join_segment_parameters_raw("/somedir/path", "rawdata"))
279 self.assertRaises(InvalidURLJoin,
280 join_segment_parameters_raw, "/somedir/path",
281 "rawdata1,rawdata2,rawdata3")
282 self.assertEquals("/somedir/path,bla,bar",
283 join_segment_parameters_raw("/somedir/path", "bla", "bar"))
284 self.assertEquals("/somedir,exist=some/path,bla,bar",
285 join_segment_parameters_raw("/somedir,exist=some/path",
286 "bla", "bar"))
287 self.assertRaises(TypeError, join_segment_parameters_raw,
288 "/somepath", 42)
289
290 def test_join_segment_parameters(self):
291 join_segment_parameters = urlutils.join_segment_parameters
292 self.assertEquals("/somedir/path",
293 join_segment_parameters("/somedir/path", {}))
294 self.assertEquals("/somedir/path,key1=val1",
295 join_segment_parameters("/somedir/path", {"key1": "val1"}))
296 self.assertRaises(InvalidURLJoin,
297 join_segment_parameters, "/somedir/path",
298 {"branch": "brr,brr,brr"})
299 self.assertRaises(InvalidURLJoin,
300 join_segment_parameters, "/somedir/path", {"key1=val1": "val2"})
301 self.assertEquals("/somedir/path,key1=val1,key2=val2",
302 join_segment_parameters("/somedir/path", {
303 "key1": "val1", "key2": "val2"}))
304 self.assertEquals("/somedir/path,key1=val1,key2=val2",
305 join_segment_parameters("/somedir/path,key1=val1", {
306 "key2": "val2"}))
307 self.assertEquals("/somedir/path,key1=val2",
308 join_segment_parameters("/somedir/path,key1=val1", {
309 "key1": "val2"}))
310 self.assertEquals("/somedir,exist=some/path,key1=val1",
311 join_segment_parameters("/somedir,exist=some/path",
312 {"key1": "val1"}))
313 self.assertEquals("/,key1=val1,key2=val2",
314 join_segment_parameters("/,key1=val1", {"key2": "val2"}))
315 self.assertRaises(TypeError,
316 join_segment_parameters, "/,key1=val1", {"foo": 42})
317
273 def test_function_type(self):318 def test_function_type(self):
274 if sys.platform == 'win32':319 if sys.platform == 'win32':
275 self.assertEqual(urlutils._win32_local_path_to_url, urlutils.local_path_to_url)320 self.assertEqual(urlutils._win32_local_path_to_url, urlutils.local_path_to_url)
@@ -412,6 +457,53 @@
412 self.assertEqual(('path/..', 'foo'), split('path/../foo'))457 self.assertEqual(('path/..', 'foo'), split('path/../foo'))
413 self.assertEqual(('../path', 'foo'), split('../path/foo'))458 self.assertEqual(('../path', 'foo'), split('../path/foo'))
414459
460 def test_split_segment_parameters_raw(self):
461 split_segment_parameters_raw = urlutils.split_segment_parameters_raw
462 self.assertEquals(("/some/path", []),
463 split_segment_parameters_raw("/some/path"))
464 self.assertEquals(("/some/path", ["tip"]),
465 split_segment_parameters_raw("/some/path,tip"))
466 self.assertEquals(("/some,dir/path", ["tip"]),
467 split_segment_parameters_raw("/some,dir/path,tip"))
468 self.assertEquals(("/somedir/path", ["heads%2Ftip"]),
469 split_segment_parameters_raw("/somedir/path,heads%2Ftip"))
470 self.assertEquals(("/somedir/path", ["heads%2Ftip", "bar"]),
471 split_segment_parameters_raw("/somedir/path,heads%2Ftip,bar"))
472 self.assertEquals(("/", ["key1=val1"]),
473 split_segment_parameters_raw(",key1=val1"))
474 self.assertEquals(("foo/", ["key1=val1"]),
475 split_segment_parameters_raw("foo/,key1=val1"))
476 self.assertEquals(("foo/base,la=bla/other/elements", []),
477 split_segment_parameters_raw("foo/base,la=bla/other/elements"))
478 self.assertEquals(("foo/base,la=bla/other/elements", ["a=b"]),
479 split_segment_parameters_raw("foo/base,la=bla/other/elements,a=b"))
480
481 def test_split_segment_parameters(self):
482 split_segment_parameters = urlutils.split_segment_parameters
483 self.assertEquals(("/some/path", {}),
484 split_segment_parameters("/some/path"))
485 self.assertEquals(("/some/path", {"branch": "tip"}),
486 split_segment_parameters("/some/path,branch=tip"))
487 self.assertEquals(("/some,dir/path", {"branch": "tip"}),
488 split_segment_parameters("/some,dir/path,branch=tip"))
489 self.assertEquals(("/somedir/path", {"ref": "heads%2Ftip"}),
490 split_segment_parameters("/somedir/path,ref=heads%2Ftip"))
491 self.assertEquals(("/somedir/path",
492 {"ref": "heads%2Ftip", "key1": "val1"}),
493 split_segment_parameters(
494 "/somedir/path,ref=heads%2Ftip,key1=val1"))
495 self.assertEquals(("/somedir/path", {"ref": "heads%2F=tip"}),
496 split_segment_parameters("/somedir/path,ref=heads%2F=tip"))
497 self.assertEquals(("/", {"key1": "val1"}),
498 split_segment_parameters(",key1=val1"))
499 self.assertEquals(("foo/", {"key1": "val1"}),
500 split_segment_parameters("foo/,key1=val1"))
501 self.assertEquals(("foo/base,key1=val1/other/elements", {}),
502 split_segment_parameters("foo/base,key1=val1/other/elements"))
503 self.assertEquals(("foo/base,key1=val1/other/elements",
504 {"key2": "val2"}), split_segment_parameters(
505 "foo/base,key1=val1/other/elements,key2=val2"))
506
415 def test_win32_strip_local_trailing_slash(self):507 def test_win32_strip_local_trailing_slash(self):
416 strip = urlutils._win32_strip_local_trailing_slash508 strip = urlutils._win32_strip_local_trailing_slash
417 self.assertEqual('file://', strip('file://'))509 self.assertEqual('file://', strip('file://'))
418510
=== 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 @@
469 return url_base + head, tail469 return url_base + head, tail
470470
471471
472def split_segment_parameters_raw(url):
473 """Split the subsegment of the last segment of a URL.
474
475 :param url: A relative or absolute URL
476 :return: (url, subsegments)
477 """
478 (parent_url, child_dir) = split(url)
479 subsegments = child_dir.split(",")
480 if len(subsegments) == 1:
481 return (url, [])
482 return (join(parent_url, subsegments[0]), subsegments[1:])
483
484
485def split_segment_parameters(url):
486 """Split the segment parameters of the last segment of a URL.
487
488 :param url: A relative or absolute URL
489 :return: (url, segment_parameters)
490 """
491 (base_url, subsegments) = split_segment_parameters_raw(url)
492 parameters = {}
493 for subsegment in subsegments:
494 (key, value) = subsegment.split("=", 1)
495 parameters[key] = value
496 return (base_url, parameters)
497
498
499def join_segment_parameters_raw(base, *subsegments):
500 """Create a new URL by adding subsegments to an existing one.
501
502 This adds the specified subsegments to the last path in the specified
503 base URL. The subsegments should be bytestrings.
504
505 :note: You probably want to use join_segment_parameters instead.
506 """
507 if not subsegments:
508 return base
509 for subsegment in subsegments:
510 if type(subsegment) is not str:
511 raise TypeError("Subsegment %r is not a bytestring" % subsegment)
512 if "," in subsegment:
513 raise errors.InvalidURLJoin(", exists in subsegments",
514 base, subsegments)
515 return ",".join((base,) + subsegments)
516
517
518def join_segment_parameters(url, parameters):
519 """Create a new URL by adding segment parameters to an existing one.
520
521 The parameters of the last segment in the URL will be updated; if a
522 parameter with the same key already exists it will be overwritten.
523
524 :param url: A URL, as string
525 :param parameters: Dictionary of parameters, keys and values as bytestrings
526 """
527 (base, existing_parameters) = split_segment_parameters(url)
528 new_parameters = {}
529 new_parameters.update(existing_parameters)
530 for key, value in parameters.iteritems():
531 if type(key) is not str:
532 raise TypeError("parameter key %r is not a bytestring" % key)
533 if type(value) is not str:
534 raise TypeError("parameter value %r for %s is not a bytestring" %
535 (key, value))
536 if "=" in key:
537 raise errors.InvalidURLJoin("= exists in parameter key", url,
538 parameters)
539 new_parameters[key] = value
540 return join_segment_parameters_raw(base,
541 *["%s=%s" % item for item in sorted(new_parameters.items())])
542
543
472def _win32_strip_local_trailing_slash(url):544def _win32_strip_local_trailing_slash(url):
473 """Strip slashes after the drive letter"""545 """Strip slashes after the drive letter"""
474 if len(url) > WIN32_MIN_ABS_FILEURL_LENGTH:546 if len(url) > WIN32_MIN_ABS_FILEURL_LENGTH: