openerp-community-reviewer team mailing list archive

[Alexandre Fayolle - camptocamp <alexandre.fayolle@xxxxxxxxxxxxxx>]

> Hello,
> 
> Thanks for the module, it is really a valuable tool.
> 
> Some comments below:
> 
> Some PEP8 error in manifest

fixed

> 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.

fixed

> 
> 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.

AFAIR in this specific case where KeyErrors are supposed to be really the exception, this version is supposed to be faster. 


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

fixed 

> 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.

done

> 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.

agreed. I'll add this. 

> 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.

I can't see why the query would fail? Do you have something in mind?

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

I'll see if I can slip this in. 
-- 
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.