Review: Needs Fixing
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
--
https://code.launchpad.net/~openerp-community/openobject-server/trunk-property-fields-using-get/+merge/67223
Your team OpenERP Community is subscribed to branch
lp:~openerp-community/openobject-server/trunk-property-fields-using-get.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-community
More help : https://help.launchpad.net/ListHelp
