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)
--
Regards,
Olemis.
