-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 09/11/2009 10:56 PM, John Dennis wrote:
I've reviewed the python binding for sss. It looks pretty good
Jakub. I
only have a few comments and most of these are style issues as opposed
to a coding or logic flaw.
Thank you for the review, the comments should be fixed in patch attached
to Simo's review of the other two patches.
* In PyErr_SetSssErrorWithMessage() you don't need to call the
PyExc_IOError constructor, instead use Py_BuildValue and use the
returned tuple as the exception value. To be honest I'm not sure what it
means to set the value of an exception to another exception which is
what it appears to be what you're doing.
Thanks, fixed
* Some of the function parameters are python objects obtained using
the
"O" format specifier for PyArg_ParseTuple. However in each instance they
can't be any python object, rather they must be List objects. You do
check the object type when you pass the object to PyList_AsStringList,
but I'd rather see the validation of the object type happen earlier and
follow existing Python convention of throwing a type exception directly
from PyArg_ParseTuple. This can easily be done by specifying the format
specifier as "O!" rather than "O". When you do this you must add an
additional argument to PyArg_ParseTuple which is a pointer to the object
type. For example
PyArg_ParseTuple(args, "O!", &PyList_Type, &py_list)
This way the error is caught before any processing is done in the
function and it then become explicitly obvious to someone reading the
code what type of object the parameter must be for correct operation. It
also means the error message will exactly match all other parameter type
errors thrown by python (consistency has a value).
Fixed.
* You're using two different memory management libraries, talloc
and
Python's. It does appear you're keeping the usage of the two separate
which is critical (however I didn't carefully check all the usage).
However mixing two different memory management libraries in one body of
code makes me nervous, the opportunity to call the wrong library
function on a pointer with disastrous consequences is always lurking. I
presume you're using talloc because sss needs it, but if you can avoid
mixing two different memory management libraries in one body of code
you'll eliminate one potential source of obscure bugs.
I know it's not ideal, but AFAIK there is no way to use tevent/confdb et
al. without talloc.
* Documentation strings. I'd rather see the docmentation strings
appear
next to the functions they are documenting, not in a far off place in
the file. That way when someone is reading the code they can see the
documentation for the function they're reading and more importantly they
can see the parameter lists and correlate that with argument extraction
(PyParseTuple). PyDoc_STRVAR() is a common convention.
Thanks, it appears that the macro is underdocumented on
docs.python.org,
so I had no idea it even existed.
* Documentation format. In IPA we've decided to use reStructured
text as
the markup (see
http://freeipa.com/page/Python_Coding_Style#Python_API_documentation).
Your docstrings look like they're in epydoc markup.
Yes, I'm used to epydoc, so I actually forgot we should be using RST. I
changed the comments to RST, but it was actually my first time using it,
so maybe I didn't use some tags I should be or vice versa. Tested
rendering the docs with "epydoc --docformat=restructuredtext
pysss.local", looks sane.
Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkquiX4ACgkQHsardTLnvCX62wCdEI+8hm7x0ElI2oL/HKWHhRNe
By8AoMKtPGQJR68JRu9cDUbA9wpt2HjX
=UILg
-----END PGP SIGNATURE-----