-----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----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
