Code review comment for lp:~barry/lazr.restful/387487-subordinate

Hi Barry.

Thanks for undertaking this effort. Since your cover letter was sparce, and the inline documentation is too, I am puzzled by the complexity of the issue described in the pub and simplicity of your solution.

> === modified file 'src/lazr/restful/publisher.py'
> --- src/lazr/restful/publisher.py 2009-03-27 04:25:34 +0000
> +++ src/lazr/restful/publisher.py 2009-07-24 21:16:28 +0000
...

> def traverseName(self, request, ob, name):
> """See `zope.publisher.interfaces.IPublication`.
> @@ -66,6 +66,12 @@
> elif IBytes.providedBy(field):
> return self._traverseToByteStorage(
> request, entry, field, name)
> + elif IObject.providedBy(field):
> + sub_entry = getattr(entry, name, None)
> + if sub_entry is None:
> + raise NotFound(ob, name, request)
> + else:
> + return sub_entry
> elif field is not None:
> return EntryField(entry, field, name)
> else:

This appears to be an easy fix for a hard problem. The problem stated in
the bug is that the suborinate object and field conflict in the name space,
but there is no conflict detection here. Are there any IObject
fields that we expect to be a field? I am sure you thought about this
since you decided this is the right solution.

> === added file 'src/lazr/restful/tests/test_navigation.py'
> --- src/lazr/restful/tests/test_navigation.py 1970-01-01 00:00:00 +0000
> +++ src/lazr/restful/tests/test_navigation.py 2009-07-24 21:16:28 +0000

...

> +class Child:
> + implements(IChildEntry)
> + schema = IChild
> +

I see trailing whitespace.

...

> +class FakePublication:
> + """A fake superclass publication."""
> + def traverseName(self, request, ob, name):
> + return 'no such name: ' + name

I am confused by this. I do not see why this is need below. Are
you testing that the FakePublication.traverseName() is never reached?

> +class NavigationPublication(WebServicePublicationMixin, FakePublication):
> + pass
> +
> +
> +class NavigationTestCase(unittest.TestCase):
> +
> + def test_toplevel_navigation(self):
> + # Test that publication can reach sub-entries.
> + publication = NavigationPublication()
> + obj = publication.traverseName(FakeRequest(), Parent(), 'child')
> + self.assertEqual(obj.one, 'one')
> +
> + def test_toplevel_navigation_without_subentry(self):
> + # Test that publication raises NotFound when subentry attribute
> + # returns None.
> + parent = Parent()
> + parent.child = None
> + publication = NavigationPublication()
> + self.assertRaises(
> + NotFound, publication.traverseName,
> + FakeRequest(), parent, 'child')