Code review comment for lp:~salgado/lazr.restful/extension-interfaces

On Wed, 2010-07-21 at 20:27 +0000, Gary Poster wrote:
> I've done a non-exhaustive review (as a supplement to Leonard's not a
> replacement), and want to share my comments so far.

Cool, thanks!

>
> Requests:
>
> - This is an important new feature for lazr.restful, and I don't see
> any instructions on how to use it. Could you add some documentation
> (doctest) to the example webservice part of the package? (As a small
> corollary, we'll want to mention something in the CHANGES/NEWS file.)

Indeed, and I also think it would be worth mentioning it in
docs/webservice-declarations.txt. What do you think?

>
> - I see that find_interfaces_and_contributors is responsible for
> raising ConflictInContributingInterfaces. I'd like to make sure that
> these errors are as helpful as they can be. Right now the exception
> just has the name and the base interface that has been modified. I'd
> also like to see the two or more interfaces that have created the
> conflict--and I'd like a test that shows that a traceback with this
> error gives the developer a nice error message telling what's gone
> wrong and where to look to address it. (A unit test of the error's
> __str__ should be sufficient.)

Something like http://paste.ubuntu.com/467438/ or would you like me to
show the error message in one of the existing unit tests?

>
> Suggestions:
>
> - for the new argument name, "contributes_to" -> "extends"? Take it
> only if you like it, but it matches nicely with "extensions," which
> you use later, for instance. If you do this, you'd want to change
> "contributors" to "extenders," I suspect.

Heh, extends is exactly what I used in the first round but after I
showed it to Leonard he suggested contributes_to. I guess I'll keep
this in mind and see if he has any other suggestions.

Cheers,

--
Guilherme Salgado <https://launchpad.net/~salgado>