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)
