lazr.restful

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

extension-interfaces
Merge into trunk

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-07-30:

On Tue, 2010-07-27 at 19:06 +0000, Leonard Richardson wrote:
> Review: Needs Fixing
> I didn't like "extends" for two reasons. First, "Foo extends bar"
> implies that 'foo' derives from 'bar', where as in this case 'bar'
> derives from 'foo'. Second, "extends" already means something in
> object-oriented programming and I didn't want to overload the term.
>
> This is really good, almost ready to land. Comments:
>
> I second Gary's suggestion about ConflictInContributingInterfaces and
> think your patch looks good.

Cool.

>
> You still need to describe this change in the NEWS file.

Done; please let me know if it looks good.

>
> I don't know how useful that TestWebServiceConfiguration refactoring
> is, but it's fine since oyu use it twice. The register_module
> refactoring is a great idea.
>
> I would like to see this feature used in an example web service (ie.
> added to examples/base or in a totally new web service of your
> design). This is less for purposes of test coverage and more to show
> how the feature would work in a real situation.

I moved some things in examples/base to make use of the new feature
(http://paste.ubuntu.com/471195/), but I see no reason why people would
use it like that in a simple interface. In fact, since the need for
this feature came from us wanting to split the implementation of some
very complex classes into smaller ones, I find it hard to think of a
good yet short example to use here. Maybe you have an idea?

While doing this I've found a bug that was causing the generated
webservice interfaces to use the doc/name of extension interfaces
instead of those from the main interface.
http://paste.ubuntu.com/471192/ is the fix

>
> Lines 802 and 803 are the same.

Not really; one of them registers ProductToHasBugs and the other
ProjectToHasBugs

>
> Some of your tests are missing explanatory comments. The stub classes

Good catch; I've just added them.

> like Product need to have docstrings saying things like "A stub class
> that publishes one string field and one read operation."

I'm not sure where to add such docstrings as the stub class really is
Product but the one that publishes things is IProduct. Also, I don't
think a docstring like that adds any value, but I could add them if you
feel strong about it.

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

On Tue, 2010-07-27 at 19:06 +0000, Leonard Richardson wrote:
> Review: Needs Fixing
> I didn't like "extends" for two reasons. First, "Foo extends bar"
> implies that 'foo' derives from 'bar', where as in this case 'bar'
> derives from 'foo'. Second, "extends" already means something in
> object-oriented programming and I didn't want to overload the term. 
> 
> This is really good, almost ready to land. Comments:
> 
> I second Gary's suggestion about ConflictInContributingInterfaces and
> think your patch looks good.

Cool.

> 
> You still need to describe this change in the NEWS file.

Done; please let me know if it looks good.

> 
> I don't know how useful that TestWebServiceConfiguration refactoring
> is, but it's fine since oyu use it twice. The register_module
> refactoring is a great idea.
> 
> I would like to see this feature used in an example web service (ie.
> added to examples/base or in a totally new web service of your
> design). This is less for purposes of test coverage and more to show
> how the feature would work in a real situation.

I moved some things in examples/base to make use of the new feature
(http://paste.ubuntu.com/471195/), but I see no reason why people would
use it like that in a simple interface.  In fact, since the need for
this feature came from us wanting to split the implementation of some
very complex classes into smaller ones, I find it hard to think of a
good yet short example to use here.  Maybe you have an idea?

> 
> Lines 802 and 803 are the same.

Not really; one of them registers ProductToHasBugs and the other
ProjectToHasBugs

> 
> Some of your tests are missing explanatory comments. The stub classes

Good catch; I've just added them.

>  like Product need to have docstrings saying things like "A stub class
> that publishes one string field and one read operation."

I'm not sure where to add such docstrings as the stub class really is
Product but the one that publishes things is IProduct.  Also, I don't
think a docstring like that adds any value, but I could add them if you
feel strong about it.

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

« Back to merge proposal