openerp-community team mailing list archive

["Olivier Dony \(OpenERP\)" <odo@xxxxxxxxxxx>]

Review: Needs Fixing technical
Hello Stefan,

Wow, very impressive work, thanks!!

Here are a few comments (line numbers are within current diff):

- we're trying to unify out docstring formats (at least for new ones, as indeed the old ones are still a mess), and trying to use RST docstrings, so that they will be readily rendered within our online doc, with the help of sphinx's autodoc module. The format is not very different, you can have a look at examples [1][2] for the format, as well as the sphinx reference [3].

- to improve the signal-to-noise ratio of the documentation, we are trying to remove all redundant documentation of the 'cr', 'uid' and 'ids' parameters. Existing docstrings have a lot of these, but we're trying to stop/remove that. 'context' can be documented when a specific key/value in it could impact the behavior of the function, otherwise it's useless too.

- any particular reason you dropped the comments about the anonymous authentication ?

- l.94, l.278, l.339: not sure if this was done on purpose, but when logging from within an except block, you can simply use logger.exception('foo') to automatically log the full exception info and trace (it will use the current exception). If you want to achieve the same with a different logging level, you can use the exc_info kwarg, as in `logger.warning('foo', exc_info=True)`. This is usually better than trying to format the exception yourself, unless you really meant it. In general, it's also best to avoid doing the string format yourself with '%', the logger will do it (and will also avoid it if no message is finally output due to the current logger level)[4].

- l.183: the explicit test on empty passwords at the start of login() is really needed, because login() does not behave like check() and does not raise in case of failure. Therefore we can't distinguish super().login() returning False because of an empty password or because of a wrong password => we need to test this case explicitly, otherwise we'll perform an anonymous auth of the _user_, something that must be prevented by applications, as discussed previously! ;-)

- l.264: 'name' seems unused now?

- l.257-226 and 324-333 seem mostly identical, how about abstracting them out also in their own overridable function, e.g. auth_user() or similar, with only the code handling the result/exception being different?


Everything else looks quite excellent, well done and thanks a lot!



[1] Simple (outdated) example: http://bazaar.launchpad.net/~openerp/openobject-server/trunk/view/head:/openerp/osv/orm.py#L2031
[2] Complicated example, showing many options http://bazaar.launchpad.net/~openerp/openobject-server/trunk/view/head:/openerp/osv/fields.py#L755
[3] Sphinx reference: http://sphinx.pocoo.org/domains.html#info-field-lists
[4] http://docs.python.org/library/logging.html#logging.Logger.debug
-- 
https://code.launchpad.net/~openerp-community/openobject-addons/stefan-therp_lp794584/+merge/63872
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-addons/stefan-therp_lp794584.