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

Revision history for this message
Gary Poster (gary) 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.

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!

« Back to merge proposal