> 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. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp