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.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-community
More help : https://help.launchpad.net/ListHelp
