Code review comment for lp:~openerp-community/openobject-server/trunk-property-fields-using-get

Hello,

Thanks for the improved properties management!

Some preliminary remarks (line numbers are line numbers of the current diff):

- When adding new docstrings, we try to match the standard RST docstring format as much as possible, as we are trying to globally improve and unify our docstrings formats (therefore it is quite important in merge props). Example:

  def squares(limit):
      """Computes the square of the first positive ints, up to ``limit``.

         :param int limit: max integer to compute (excluded)
         :return: map of integers to their square
         :rtype: dict
      """
      return dict([(x, x**2) for x in xrange(limit)])

- l.40 of the diff, in _get_defaults, what is the weird loop? That's always a one-item list, so there is no reason for looping at all, nor for making the list in the first place?

- in _fnct_read(), the test `isinstance(x, orm.browse_record)` is repeated many times within the inner loops, while it should only be in the outer loop, and just once. It also seems incorrect most of the times, as it tests the default value type, but the final value is the one that is used. What if there is no default but a real value is indeed set?
Looks like this should instead be based on the property's 'type' column, shouldn't it?
The inner loops could be quite simplified too.

- l.108 in _fnct_read(): name_get() must explicitly be called as UID 1, because it is quite possible for the target record to be a record the user is not allowed to access, and seeing the name_get() is still allowed (the permission of the parent object is the one that counts). Didn't you notice the explicit comment about that in the code? ;-)

- similarly, the code had an explicit check for the existence of the target records of m2o property values, because there is not integrity management for properties done by the database. Removing that code will certainly cause regressions, unless you have taken care of it somewhere else? BTW this test must be done as UID 1 for the same reason as above. Didn't you notice the explicit comment about that stuff in the code? ;-)

- about the utility function get_and_sort_by_field(): it does not strike me as a frequent use case, so perhaps it's better to make a specific function that gives the ids of records grouped by company_id? It could be based on this generic function, but perhaps the purpose would be more clear...
  + BTW, was naming a dict value "key" a sort of joke? (l.136) ;-)
  + l.139 will not work as "set_field" is not defined, plus these 3 lines should be replaced by:
       res.setdefault(key,[]).append(item['id'])

A more complete review will probably follow with some real testing of the code.

Thanks!

-- odo, volunteer framework reviewing monkey