openerp-community-reviewer team mailing list archive

[Nicolas Bessi - Camptocamp <nicolas.bessi@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Hello,

Thanks for the module, it is really a valuable tool.

Some comments below:

Some PEP8 error in manifest 

728+   name = '%s.%s' % (cls.__module__, cls.__name__)
There is a logical risk that cls is not initialized if it is not an old/new style class. 

729	+            try:
730	+                counts[name] += 1
731	+            except KeyError:
732	+                counts[name] = 1

Just a matter of taste but I prefer the use of a defaultdict or "if name in counts .. else .."
Maybe there is a performance advantage to using try catch approach I do not know. 

I think the TODO to log comment is not up to date.

Some test are missing even if they will just ensure that code will not crash.
I agree that testing returned values is not reasonable here. 

Nice to Have:

It would be nice to have a parameter to disable process logging (just in case ;P) in config without having to uninstall the module.

Maybe we can DRY function that does SQL queries to measure and add some exception management, If one query fail It would be nice to have other monitoring data saved.

Not really useful but we may add an abstract model parent of server.monitor.table.xxx

Thanks again

Regards

Nicolas
-- 
https://code.launchpad.net/~camptocamp/server-env-tools/7.0-monitoring/+merge/215138
Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools.