Code review comment for lp://staging/~gary/lazr.restful/fix-webservice-testing-infrastructure

On Aug 25, 2009, at 8:29 AM, Leonard Richardson wrote:

> Review: Needs Information
>> Fixes testing infrastructure and packaging problems.
>
> I have two comments.
>
> 1. Why the heck is 'roman' a dependency of lazr.restful? It's not in
> the code and it's such a random thing to need.

I was surprised too, but my tests failed in a clean Python (that is, a
vanilla Python with only the standard library) without it. That said,
I investigated further after you asked. It turns out that docutils
tries to do something tricky, and my version of docutils had broken
the trick. I rebuilt docutils and we're all good. Therefore, I
removed that indirect dependency from the list.

> 2. This comment isn't terribly helpful.
>
> 70 + # We want the initial slash of the path, so we subtract 1 from
> 71 + # the base_url's len.
>
> I'd prefer something like:
>
> # We want just one character of the base_url (the slash that begins
> the path), so we cut
> # off the length of the base_url minus one character.

Yes, good improvement. I took it a little further, as you can see in
the incremental diff below.

Thanks

Gary

=== modified file 'setup.py'
--- setup.py 2009-08-20 04:18:09 +0000
+++ setup.py 2009-08-25 13:46:54 +0000
@@ -61,7 +61,6 @@
          'lazr.enum',
          'lazr.lifecycle',
          'lazr.uri',
- 'roman',
          'setuptools',
          'simplejson',
          'van.testing',

=== modified file 'src/lazr/restful/testing/webservice.py'
--- src/lazr/restful/testing/webservice.py 2009-08-20 04:18:09 +0000
+++ src/lazr/restful/testing/webservice.py 2009-08-25 14:04:09 +0000
@@ -211,8 +211,9 @@
                          'preceding slash?)',
                          path_or_url)
              else:
- # We want the initial slash of the path, so we
subtract 1 from
- # the base_url's len.
+ # The base_url ends with a slash. We want that slash
to begin
+ # the path, so we cut off the length of the base_url
minus one
+ # character.
                  full_url = path_or_url[len(base_url)-1:]
          full_headers = {'Host': self.domain}
          if headers is not None: