lazr.restful

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

extension-interfaces
Merge into trunk

Revision history for this message

Gary Poster (gary) wrote on 2010-07-23:

On Jul 22, 2010, at 6:28 AM, Guilherme Salgado wrote:

> 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?

Looks perfect, thank you.

>
>>
>> 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.

Understood.

Leonard will not be back today. I'll discuss on IRC how we should proceed.

Gary

On Jul 22, 2010, at 6:28 AM, Guilherme Salgado wrote:

> 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?

Looks perfect, thank you.

> 
>> 
>> 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.

Understood.

Leonard will not be back today.  I'll discuss on IRC how we should proceed.

Gary

« Back to merge proposal