Merge lp:~salgado/lazr.restful/extension-interfaces into lp:lazr.restful

This branch is slow going but I have one suggestion. Can you test this assertion?

+ assert name in orig_iface, (
+ "Could not find interface where %s is defined" % name)

I don't see how it could be triggered. But maye the point is that it never is triggered?

On Fri, 2010-07-16 at 15:36 +0000, Leonard Richardson wrote:
> This branch is slow going but I have one suggestion. Can you test this assertion?
>
> + assert name in orig_iface, (
> + "Could not find interface where %s is defined" % name)
>
> I don't see how it could be triggered. But maye the point is that it never is triggered?

As we discussed at the Epic, I don't think we can trigger that assertion
without introducing a bug somewhere.

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

I've done a non-exhaustive review (as a supplement to Leonard's not a replacement), and want to share my comments so far.

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

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

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.

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>

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?

+1

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

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.

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

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.

Lines 802 and 803 are the same.

Some of your tests are missing explanatory comments. The stub classes like Product need to have docstrings saying things like "A stub class that publishes one string field and one read operation."

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>

Other than the things I'd mentioned in my previous comment, I also added a new unit test to show that contributing interfaces are not exported by themselves together with a short explanation of contributing interfaces on webservice-declarations.txt

This is really a very nice branch. Thank you.

I hate to not give it a final go-ahead, but I do have one last request.

= Requests =

You said:
> Leonard said:
> > 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?

I'd argue that the central story is reuse. That reuse might be to factor out a simpler webservice that can be reused elsewhere (which is actually what is driving your work, AIUI) or it might be to extend a simpler webservice (which you might end up doing as well). I think you could easily explain a change in this way.

If you didn't already have an example, I might try to think up some scenario, like another branch of a company that wanted to add, say, the idea of a recipe contributor, or wanted to add the idea of favorite recipes or recipe ratings. That particular idea might be undesirable for the imaginary people using the base website.

What you have already might work though. While it's hard to imagine someone not wanting search, perhaps other teams have a more complicated search method. You are sharing/reusing the base webservice, and you just want to implement a very simple search. The other team works from the same base code, but extends the cookbook with a much more complex search method that also incorporates ideas that you don't need and don't want, like contributors, favorites, and/or ratings.

Can you use your patch and then add an explanation with an imaginary scenario like the one I gave above?

= Small suggestions =

In generate_entry_adapters (src/lazr/restful/declarations.py) maybe add a comment explaining that the update ("fields.update(getFields(iface).items())") is not destructive because we have already verified that there are no overlaps in find_interfaces_and_contributors.

= Thoughts =

AFAICT, saying that an exported interface contributes to another interface effectively exports that other interface even if it were not initially marked to be exported. I don't love that, but I don't think it is a big deal, and I'm not going to worry about it.

Product docstrings: I'm fine with leaving them out as you suggest.

NEWS looks good. Thank you!

On Tue, 2010-08-03 at 18:11 +0000, Gary Poster wrote:
> This is really a very nice branch. Thank you.
>
> I hate to not give it a final go-ahead, but I do have one last request.
>
> = Requests =
>
> You said:
> > Leonard said:
> > > 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?
>
> I'd argue that the central story is reuse. That reuse might be to
> factor out a simpler webservice that can be reused elsewhere (which is
> actually what is driving your work, AIUI) or it might be to extend a
> simpler webservice (which you might end up doing as well). I think
> you could easily explain a change in this way.

Well, what we really want is to be able to move attributes/methods out
of existing interfaces and into adapters while still publicizing a
single entry comprising what's exported by the changed interface and all
of its adapters. For the reuse story one can use mix-ins as long as
it's ok to have the exported interface inherit from a bunch of others
(as we currently do).

>
> If you didn't already have an example, I might try to think up some
> scenario, like another branch of a company that wanted to add, say,
> the idea of a recipe contributor, or wanted to add the idea of
> favorite recipes or recipe ratings. That particular idea might be
> undesirable for the imaginary people using the base website.
>
> What you have already might work though. While it's hard to imagine
> someone not wanting search, perhaps other teams have a more
> complicated search method. You are sharing/reusing the base
> webservice, and you just want to implement a very simple search. The
> other team works from the same base code, but extends the cookbook
> with a much more complex search method that also incorporates ideas
> that you don't need and don't want, like contributors, favorites,
> and/or ratings.
>

After letting this marinate on my head for a while longer, I thought of
creating an IHasComments interface that contributes a 'comments'
attribute to IRecipe, as if there was a separate web service which wants
to allow its users to comment on recipes
(http://paste.ubuntu.com/472821/). How does that sound to you?

I've also modified one of the tests for the base example to show the
comments, but that uncovered a bug in the current implementation:
EntryResource.redacted_fields will look for the 'comments' attribute on
the IRecipe object directly instead of through its IHasComments adapter,
and that causes a ForbiddenAttribute. I think I could change
redacted_fields to adapt the context into the field'...

http://paste.ubuntu.com/473132/ fixes the bug in EntryResource.redacted_fields. It basically stores, as a class variable in the generated adapter, a dict mapping every field of that adapter to the interface where that field came from. That way we can make redacted_fields use the checker for the correct interface for every field.

On Aug 3, 2010, at 5:46 PM, Guilherme Salgado wrote:

> On Tue, 2010-08-03 at 18:11 +0000, Gary Poster wrote:
>> This is really a very nice branch. Thank you.
>>
>> I hate to not give it a final go-ahead, but I do have one last request.
>>
>> = Requests =
>>
>> You said:
>>> Leonard said:
>>>> 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?
>>
>> I'd argue that the central story is reuse. That reuse might be to
>> factor out a simpler webservice that can be reused elsewhere (which is
>> actually what is driving your work, AIUI) or it might be to extend a
>> simpler webservice (which you might end up doing as well). I think
>> you could easily explain a change in this way.
>
> Well, what we really want is to be able to move attributes/methods out
> of existing interfaces and into adapters while still publicizing a
> single entry comprising what's exported by the changed interface and all
> of its adapters. For the reuse story one can use mix-ins as long as
> it's ok to have the exported interface inherit from a bunch of others
> (as we currently do).

Right, but the mix-in approach means a lot more work for small changes. I could be wrong, but with the mix-in class approach, you have to make your own separate registration of *everything* because you don't want one interface, you want another one. There are examples of this kind of annoyance in the webservice package. With your feature, you can reuse the entirety of a webservice, including its registrations, and then register your own things in addition. That's much easier.

Maybe I'm missing something, but that's how I see it right now.

>
>>
>> If you didn't already have an example, I might try to think up some
>> scenario, like another branch of a company that wanted to add, say,
>> the idea of a recipe contributor, or wanted to add the idea of
>> favorite recipes or recipe ratings. That particular idea might be
>> undesirable for the imaginary people using the base website.
>>
>> What you have already might work though. While it's hard to imagine
>> someone not wanting search, perhaps other teams have a more
>> complicated search method. You are sharing/reusing the base
>> webservice, and you just want to implement a very simple search. The
>> other team works from the same base code, but extends the cookbook
>> with a much more complex search method that also incorporates ideas
>> that you don't need and don't want, like contributors, favorites,
>> and/or ratings.
>>
>
> After letting this marinate on my head for a while long...

For posterity...

On IRC I mentioned to Salgado that in http://paste.ubuntu.com/473132/ it looks like we are discarding a checker in the change starting in line 23. He gave me http://paste.ubuntu.com/473195/ which was what I was looking for.

On Wed, 2010-08-04 at 18:09 +0000, Gary Poster wrote:
> >
> > After letting this marinate on my head for a while longer, I thought of
> > creating an IHasComments interface that contributes a 'comments'
> > attribute to IRecipe, as if there was a separate web service which wants
> > to allow its users to comment on recipes
> > (http://paste.ubuntu.com/472821/). How does that sound to you?
>
> I like it.
>
> You *could* show what I was just talking about above, if you wanted to, by setting up a separate webservice that added comments to the original webservice. It ought to be super easy--include the configuration of the original webservice plus your extra bits. If it's not easy then that might even be construed as a problem.
>
> But that's not a requirement for this to land.
>

I've tried that by doing the same thing that the other examples do, but
it didn't work: http://paste.ubuntu.com/473253/

http://paste.ubuntu.com/473254/ has my changes, but I've also pushed
them to my branch. Any idea why it's not working?

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

Awesome! Thank you.