Mailing-List: contact bloodhound-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: bloodhound-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of olemis@gmail.com designates
 74.125.82.43 as permitted sender)
MIME-Version: 1.0
Date: Thu, 14 Jun 2012 00:03:06 -0500
Message-ID: 
 <CAGMZAuOmsKA=PPj+HvyvXADcboWg5xGmBE=H_N8RJ-Yi2+y74g@mail.gmail.com>
Subject: [RFC] [multiproduct] Review #1
From: Olemis Lang <olemis@gmail.com>
To: bloodhound-dev <bloodhound-dev@incubator.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

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* (<=3D 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
        (<=3D 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 wi=
th
     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.ProductPermissio=
ns`
     class with `multiproduct.model.MultiProductEnvironmentProvider` (... o=
r
     `multiproduct.api.MultiProductSystem` especially if =A77 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)

--=20
Regards,

Olemis.