Patch: Add a filter list to the model graph wizard

With this patch it is possible to filter given names of models (like "account.account", or nameparts like ".template") from the creation of the graph. You can set a higher level for a deep model structure, and filter out models you don't like to see in the final graph.

[ced1, 2009-12-06 13:22:41]

http://codereview.appspot.com/165069/diff/1/2
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1/2#newcode360
trytond/ir/model.py:360: filter_ = fields.Text('Namespace Filter', help=("Remove
"
Why not use filter?
It is not a reserved word.

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
Why not use a regexp?
It is more powerful.

[yangoon1, 2009-12-06 13:24:06]

http://codereview.appspot.com/165069/diff/1/2
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1/2#newcode364
trytond/ir/model.py:364: "account. \nparty.address \nstock \ninvoice"))
Proposal:
Don't display models, that match the given filter expressions.
Note:
Filter expressions have to be inserted on separate lines.
Example: ...

http://codereview.appspot.com/165069/diff/1/2#newcode454
trytond/ir/model.py:454: if expression == '':
Filter expressions could be prepared before entering loop for model in models:
to prevent unneeded iteration.

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
Why not just:
if model in expressions:
?

[udono, 2009-12-06 14:18:56]

http://codereview.appspot.com/165069/diff/1/2
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1/2#newcode360
trytond/ir/model.py:360: filter_ = fields.Text('Namespace Filter', help=("Remove
"
On 2009/12/06 13:22:41, ced wrote:
> Why not use filter?
> It is not a reserved word.

Done.

http://codereview.appspot.com/165069/diff/1/2#newcode364
trytond/ir/model.py:364: "account. \nparty.address \nstock \ninvoice"))
On 2009/12/06 13:24:06, yangoon wrote:
> Proposal:
> Don't display models, that match the given filter expressions.
> Note:
> Filter expressions have to be inserted on separate lines.
> Example: ...

Done.

http://codereview.appspot.com/165069/diff/1/2#newcode454
trytond/ir/model.py:454: if expression == '':
On 2009/12/06 13:24:06, yangoon wrote:
> Filter expressions could be prepared before entering loop for model in models:
> to prevent unneeded iteration.
> 
> 

Done.

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
On 2009/12/06 13:22:41, ced wrote:
> Why not use a regexp?
> It is more powerful.
I thought of it, but decided that it is complicate, because the user needs to
know regexp. I think using find is more KISS.

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
On 2009/12/06 13:24:06, yangoon wrote:
> Why not just:
> if model in expressions:
> ?

Done.

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
On 2009/12/06 13:24:06, yangoon wrote:
> Why not just:
> if model in expressions:
> ?

Done.

[ced1, 2009-12-06 14:54:43]

http://codereview.appspot.com/165069/diff/1/2
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1/2#newcode456
trytond/ir/model.py:456: if model.model.find(expression) > -1:
On 2009/12/06 14:18:56, udo.spallek wrote:
> On 2009/12/06 13:22:41, ced wrote:
> > Why not use a regexp?
> > It is more powerful.
> I thought of it, but decided that it is complicate, because the user needs to
> know regexp. I think using find is more KISS. 

I don't agree.
Per example you can not have only "^account." with find, you will have
analytic.account.
Simple ok but not simplicity.

[udono, 2009-12-06 16:00:21]

On 2009/12/06 14:54:43, ced wrote:
> http://codereview.appspot.com/165069/diff/1/2
> File trytond/ir/model.py (right):
> 
> http://codereview.appspot.com/165069/diff/1/2#newcode456
> trytond/ir/model.py:456: if model.model.find(expression) > -1:
> On 2009/12/06 14:18:56, udo.spallek wrote:
> > On 2009/12/06 13:22:41, ced wrote:
> > > Why not use a regexp?
> > > It is more powerful.
> > I thought of it, but decided that it is complicate, because the user needs
to
> > know regexp. I think using find is more KISS. 
> 
> I don't agree.
> Per example you can not have only "^account." with find, you will have
> analytic.account.
> Simple ok but not simplicity.
yes, you are right. Done.

[ced1, 2009-12-06 16:09:29]

http://codereview.appspot.com/165069/diff/1008/1009
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1008/1009#newcode364
trytond/ir/model.py:364: is_regexp = fields.Boolean("Use Regular Expressions",
help=("The given "
Use only regexp.
Mixing both is worth.

http://codereview.appspot.com/165069/diff/1008/1009#newcode457
trytond/ir/model.py:457: expressions = [item for item in filter.split('\n') if
item != '']
Use one line regexp.
Otherwise it is not clear how it works if it must match both or just one etc.

http://codereview.appspot.com/165069/diff/1008/1009#newcode461
trytond/ir/model.py:461: if [expr for expr in expressions if re.search(expr,
model.model)]:
You should compile the regexp for performance.

[udono, 2009-12-06 16:42:47]

http://codereview.appspot.com/165069/diff/1008/1009
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1008/1009#newcode457
trytond/ir/model.py:457: expressions = [item for item in filter.split('\n') if
item != '']
On 2009/12/06 16:09:29, ced wrote:
> Use one line regexp.
> Otherwise it is not clear how it works if it must match both or just one etc.

I don't understand what to do. Do you mean I should use fields.Char instead of
fields.Text for the filter?

[ced1, 2009-12-06 22:21:13]

http://codereview.appspot.com/165069/diff/1008/1009
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/1008/1009#newcode457
trytond/ir/model.py:457: expressions = [item for item in filter.split('\n') if
item != '']
On 2009/12/06 16:42:47, udo.spallek wrote:
> On 2009/12/06 16:09:29, ced wrote:
> > Use one line regexp.
> > Otherwise it is not clear how it works if it must match both or just one
etc.
> 
> I don't understand what to do. Do you mean I should use fields.Char instead of
> fields.Text for the filter?

Yes with regexp you can have "and" and "or" operator in one regexp.

[udono, 2009-12-08 14:45:15]

On 2009/12/06 22:21:13, ced wrote:
> http://codereview.appspot.com/165069/diff/1008/1009
> File trytond/ir/model.py (right):
> http://codereview.appspot.com/165069/diff/1008/1009#newcode457
> trytond/ir/model.py:457: expressions = [item for item in filter.split('\n') if
> item != '']
> On 2009/12/06 16:42:47, udo.spallek wrote:
> > On 2009/12/06 16:09:29, ced wrote:
> > > Use one line regexp.
> > > Otherwise it is not clear how it works if it must match both or just one
> etc.
> > I don't understand what to do. Do you mean I should use fields.Char instead
of
> > fields.Text for the filter?
> Yes with regexp you can have "and" and "or" operator in one regexp.
Ups, last message lost... again:
I thought about that, and in general you are right. Just supporting RE is a
better way for the filter. But I disagree with the on-line char field as UI for
the RE. My filter terms are very long, and Its hard to edit them in a single
line. What do you think about:
http://docs.python.org/library/re.html#re.VERBOSE

[ced1, 2009-12-08 14:54:06]

On 2009/12/08 14:45:15, udo.spallek wrote:
> On 2009/12/06 22:21:13, ced wrote:
> > http://codereview.appspot.com/165069/diff/1008/1009
> > File trytond/ir/model.py (right):
> > http://codereview.appspot.com/165069/diff/1008/1009#newcode457
> > trytond/ir/model.py:457: expressions = [item for item in filter.split('\n')
if
> > item != '']
> > On 2009/12/06 16:42:47, udo.spallek wrote:
> > > On 2009/12/06 16:09:29, ced wrote:
> > > > Use one line regexp.
> > > > Otherwise it is not clear how it works if it must match both or just one
> > etc.
> > > I don't understand what to do. Do you mean I should use fields.Char
instead
> of
> > > fields.Text for the filter?
> > Yes with regexp you can have "and" and "or" operator in one regexp.
> Ups, last message lost... again:
> I thought about that, and in general you are right. Just supporting RE is a
> better way for the filter. But I disagree with the on-line char field as UI
for
> the RE. My filter terms are very long, and Its hard to edit them in a single
> line. What do you think about:
> http://docs.python.org/library/re.html#re.VERBOSE

Why not if the help make reference to it.

[udono, 2009-12-08 16:01:58]

hey,
I uploaded patchset 4 with all your ideas implemented. Please give the help
message and the general functioning a look.
Maybe you have another good example for a regexp, or some better words for the
description?!

Cheers 
Udo

[ced1, 2009-12-08 16:51:14]

http://codereview.appspot.com/165069/diff/12/2004
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/12/2004#newcode371
trytond/ir/model.py:371: ))
"Don't display models that match the Python Regular Expression."
It is clear, simple and give all the needed information.

http://codereview.appspot.com/165069/diff/12/2004#newcode443
trytond/ir/model.py:443: filter_re = re.compile(filter, re.VERBOSE)
Don't compile filter at each iteration.
Pass it as param to fill_graph

[udono, 2009-12-09 09:14:56]

On 2009/12/08 16:51:14, ced wrote:
> http://codereview.appspot.com/165069/diff/12/2004
> File trytond/ir/model.py (right):
> 
> http://codereview.appspot.com/165069/diff/12/2004#newcode371
> trytond/ir/model.py:371: ))
> "Don't display models that match the Python Regular Expression."
> It is clear, simple and give all the needed information.
Yes, thanks. Done.

> http://codereview.appspot.com/165069/diff/12/2004#newcode443
> trytond/ir/model.py:443: filter_re = re.compile(filter, re.VERBOSE)
> Don't compile filter at each iteration.
> Pass it as param to fill_graph
Done. See Patch Set 5

[ced1, 2009-12-09 09:26:17]

http://codereview.appspot.com/165069/diff/2007/2008
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/2007/2008#newcode362
trytond/ir/model.py:362: "E.g. Filter all models with...\n"
I find that Python Regular Expressions is enough as help.
We don't need of examples.

http://codereview.appspot.com/165069/diff/2007/2008#newcode369
trytond/ir/model.py:369: "See http://docs.python.org/howto/regex.html for more
examples."
Putting url in help is not very usable as they are not clickable nor copyable.

http://codereview.appspot.com/165069/diff/2007/2008#newcode438
trytond/ir/model.py:438: filter=False, context=None):
Putting a doc string will be better

[yangoon1, 2009-12-09 11:08:01]

http://codereview.appspot.com/165069/diff/2007/2008
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/2007/2008#newcode360
trytond/ir/model.py:360: filter = fields.Text('Filter Lines', help=("Don't
display models, "
fields.Text('Filter', ...

http://codereview.appspot.com/165069/diff/2007/2008#newcode370
trytond/ir/model.py:370: ))
Proposal:
Entering a Python Regular Expression will exclude models from being displayed.

[yangoon1, 2009-12-09 11:48:34]

http://codereview.appspot.com/165069/diff/2007/2008
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/2007/2008#newcode370
trytond/ir/model.py:370: ))
On 2009/12/09 11:08:01, yangoon wrote:
> Proposal:
> Entering a Python Regular Expression will exclude models from being displayed.

Better:
Entering a Python Regular Expression will exclude matching models from being
displayed.

[udono, 2009-12-11 12:18:26]

upload new patch set

[ced1, 2009-12-11 12:31:47]

http://codereview.appspot.com/165069/diff/4001/4002
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/4001/4002#newcode361
trytond/ir/model.py:361: "Regular Expression will exclude models from being "
... models from the graph.

http://codereview.appspot.com/165069/diff/4001/4002#newcode406
trytond/ir/model.py:406: if datas['form']['filter'] is False:
filter could be also empty string so test must be:

if not datas['form']['filter']:

http://codereview.appspot.com/165069/diff/4001/4002#newcode426
trytond/ir/model.py:426: context=context)
You can put context on the same line then filter, no?

http://codereview.appspot.com/165069/diff/4001/4002#newcode431
trytond/ir/model.py:431: filter=False, context=None):
Is it not better to have None as default value for filter?

http://codereview.appspot.com/165069/diff/4001/4002#newcode436
trytond/ir/model.py:436: :param user: Tryton user
Why not use the same description than in other part of the code:
the database cursor
the user id

http://codereview.appspot.com/165069/diff/4001/4002#newcode437
trytond/ir/model.py:437: :param models: Models to show in graph
a BrowserRecordList of ir.model

http://codereview.appspot.com/165069/diff/4001/4002#newcode438
trytond/ir/model.py:438: :param graph: Object of class pydot.Dot()
A pydot.Graph

http://codereview.appspot.com/165069/diff/4001/4002#newcode441
trytond/ir/model.py:441: E.g. Filter all models with...
Why putting again here the examples.
The reader of the code will know what is Python regex.

http://codereview.appspot.com/165069/diff/4001/4002#newcode452
trytond/ir/model.py:452: :return: Nothing. This method works directly on the
given graph.
If it returns nothing, don't put :return:

http://codereview.appspot.com/165069/diff/4001/4002#newcode477
trytond/ir/model.py:477: if re.search(filter, model.model):
Could be in one if statement:
if filter and re.search(filter, model.model):

[udono, 2009-12-11 12:55:22]

upload new patch set

[ced1, 2009-12-11 13:02:57]

http://codereview.appspot.com/165069/diff/5003/5004
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/5003/5004#newcode406
trytond/ir/model.py:406: filter = False
More logical: filter = None

http://codereview.appspot.com/165069/diff/5003/5004#newcode437
trytond/ir/model.py:437: :param filter: a Python Regular Expression to filter
specific models
It is a compiled regular expression objects.

[udono, 2009-12-11 13:38:01]

new patch set uploaded

[ced1, 2009-12-11 13:40:12]

Ok for me

[udono, 2009-12-11 19:08:09]

Add new patch. Small change in docstring, thanks to yangoon.

[ced1, 2009-12-11 19:09:17]

Still ok

[ced1, 2009-12-17 10:46:54]

http://codereview.appspot.com/165069/diff/4010/5012
File trytond/ir/model.py (right):

http://codereview.appspot.com/165069/diff/4010/5012#newcode361
trytond/ir/model.py:361: "Regular Expression will exclude matching models from
the graph."))
There is no need of () around help

[udono, 2009-12-17 11:46:07]

On 2009/12/17 10:46:54, ced wrote:
> http://codereview.appspot.com/165069/diff/4010/5012
> File trytond/ir/model.py (right):
> 
> http://codereview.appspot.com/165069/diff/4010/5012#newcode361
> trytond/ir/model.py:361: "Regular Expression will exclude matching models from
> the graph."))
> There is no need of () around help
But when not using () I need to use "..." + \
For me () is nicer and more clear

[ced1, 2009-12-17 11:48:55]

On 2009/12/17 11:46:07, udo.spallek wrote:
> On 2009/12/17 10:46:54, ced wrote:
> > http://codereview.appspot.com/165069/diff/4010/5012
> > File trytond/ir/model.py (right):
> > 
> > http://codereview.appspot.com/165069/diff/4010/5012#newcode361
> > trytond/ir/model.py:361: "Regular Expression will exclude matching models
from
> > the graph."))
> > There is no need of () around help
> But when not using () I need to use "..." + \
> For me () is nicer and more clear

You are already inside a statement so you don't need any thing.

[udono, 2009-12-17 12:55:20]

Yes, you are right ced, parentheses are not needed. Thanks, new patch uploaded.

[ced1, 2009-12-17 13:01:15]

Ok