incubator-bloodhound-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Martin <gary.mar...@wandisco.com>
Subject Re: [RFC] [multiproduct] Review #1
Date Thu, 14 Jun 2012 12:54:13 GMT
Hi Olemis,

A bit too much in that message to comment sensibly in a single email but 
I will try to cover some points. The first part is a particularly 
confusing as I do not see the common module in 
bloodhound_multiproduct/multiproduct, I wonder if you are looking at the 
right code.

Meanwhile I didn't follow the p prefix idea but instead decided on 
req.path_info.startswith('/products') so that it is more obvious what it 
is for. I don't mind variations on that but I think that /p is not 
enough to retain clarity. Even if google use it.

>   2- IMO import statements in `multiproduct.__init__` should be absolute

I have no problem with that change.

>   3- I wonder whether it'd be a good idea to

Possibly but it could be overkill if there are not going to be that many 
additional models to support.

>   4- Product table key is ['prefix', 'name'] . Why ?

Well, certainly both are considered unique. We can look at whether this 
is correct usage.

>   5- It's cool to have models ! :)

I agree it would probably be better to use a pre-existing ORM - I would 
quite like to see the whole of bloodhound including trac to use such an 
approach so that more of the system can be taken a consistent step away 
from raw SQL. I don't know if I am on my own in liking good ORMs of 
course which is why I have not made too much of it. I do have a secret 
desire to be using django's model and query system though!

I'll have to consider the rest later but there is not a huge amount to 
actually disagree with. Things do need moving around and the code has 
languished a little while I have been distracted by other concerns.

I think we are placing a higher priority on getting the basic interface 
right than getting multi-product fully working (product namespaces, 
permissions etc). I can see that work slipping to the next milestone if 
we are not able to get more people interested in working with us and up 
to speed in the short term.

Cheers,
     Gary


On 06/14/2012 06:03 AM, Olemis Lang wrote:
> Hi !
> :)
>
> I've been reviewing the code in multiproduct plugin these days . I
> have some doubts | comments | ... I thought I'd share them with you .
>
>    1- I've noticed that function `multiproduct.common.match_product_path` is not
>       used anywhere else . What is it for ?
>       a) After a while I found
>          `multiproduct.ticket_web_ui.ProductTicketModule.match_request`
>          method and I confirmed my intuition was correct.
>          As far as I can say *NOW* (<= i.e. I can change my mind if
>          there're strong arguments supporting that approach ;)
>          that's something I do not recommend .
>          `IRequestHandler.match_request` should decide quickly
>          (<= and that means **really** quick) whether the
>          handler will process the request or not.
>          I even notice the execution of a SQL
>          select statement . IMO that's far too much to belong in
>          `match_request` method. Besides that approach may lead
>          to collisions with paths matched by other plugins .
>          That's the reason why I suggested in t.e.o site , and
>          trac-dev ML to use ''/p/'' prefix similar to Google
>          Code (which is short and simple) to reduce the scope
>          of web request to the context of a particular
>          product / project. Sample ticket URL would be e.g.
>          http://server.com/path/to/bh/p/my_product/ticket/36
>          and (initially ;) may be matched using a simple regex
>          like ` r'^/p/(?P<pid>[^/]+/ticket/(?P<tid>\d+)$)' `.
>    2- IMO import statements in `multiproduct.__init__` should be absolute i.e.
>       `from multiproduct.model import MultiProductEnvironmentProvider`
>       instead of `from model import MultiProductEnvironmentProvider`
>    3- I wonder whether it'd be a good idea to
>       a) ... populate `MultiProductEnvironmentProvider.SCHEMA` considering
>          `SomeModel.__meta__` (e.g. `SomeModel` may be `Product` ,
>          `ProductResourceMap`, ...)
>       b) ... make `MultiProductEnvironmentProvider` capable of discovering
>          existing classes dynamically i.e. in such a way that it won't be
>          necessary to update that class if new models are introduced .
>    4- Product table key is ['prefix', 'name'] . Why ?
>    5- It's cool to have models ! :)
>       Is it a good idea to consider using ORMs like SqlAlchemy [1]_
>       (... or maybe other ...) ?
>    6- IMO we should add multi-product version to '''System Information'''
>       table in ''About Trac'' page (i.e. http://server.com/path/to/bh/about )
>    7- I also suggest the following changes in files (relative to
>       ''multiproduct'' folder) :
>
>       || Current file path || Suggestions                    ||
>       ||-------------------||--------------------------------||
>       || product_admin.py  || admin/product.py , admin.py    ||
>       || ticket_web_ui.py  || web_ui/ticket.py               ||
>       || web_ui.py         || web_ui/__init__.py             ||
>       ||<create new>       || api.py                         ||
>
>       ... and the following simple refactorings to move classes
>       and change their names :
>
>       || From                                  || To
>                   ||
>       ||---------------------------------------||----------------------------------------||
>       || model.MultiProductEnvironmentProvider ||
> api.MultiProductSystem                 ||
>       || ticket_web_ui.ProductTicketModule     ||
> web_ui.ticket.MultiProductTicketModule ||
>
>    8- IMO `multiproduct.model.MultiProductEnvironmentProvider` should
>       not implement `trac.web.api.ITemplateProvider` as it is not related with
>       request handling at all . It doesn't even make use of any ''Genshi''
>       template included in the plugin. Usually this is the kind of things
>       defined in `web_ui` modules .
>    9- I suggest merging current `multiproduct.product_admin.ProductPermissions`
>       class with `multiproduct.model.MultiProductEnvironmentProvider` (... or
>       `multiproduct.api.MultiProductSystem` especially if ยง7 is ok ;).
>    10- There's a lot of great stuff in multi product plugin . IMO it
>        deserves some test cases , so we should have a ticket for that
>        scheduled for RC1 ... or some other milestone ... that's
>        something we should decide ;)
>
> PS: I could not finished ... so maybe there's a second message on this
> subject ;)
>
> .. [1] Trac SQLAlchemy Bridge
>         (http://trac-hacks.org/wiki/TracSqlAlchemyBridgeIntegration)
>


Mime
View raw message