Code review comment for lp:~doanac/utah/bug1158743

Revision history for this message
Javier Collado (javier.collado) wrote :

Thanks for the update (specially for the test cases).

I've got a couple of commments:

- Exceptions

When getting the process object, TypeError and ValueError exceptions are
caught:
except (psutil.error.NoSuchProcess, TypeError, ValueError):

However, I think they shouldn't be caught since I'd like to get an exception
when I don't pass the right parameter type as in:
pid_in_use('string')

Otherwise, I misuse is not noticed by the end user.

- Documentation

I've seen you've made the effor to document everything. However the
documentation strings aren't pep257 (http://www.python.org/dev/peps/pep-0257/)
compliant and they don't follow the sphinx markup. If you could fix those
problems as well (just on the new code), that would be great.

To check documentation strings, I use the following tool and works quite well:
https://pypi.python.org/pypi/pep257/

With regard sphinx markup, please have a look at:
http://sphinx-doc.org/domains.html#info-field-lists

In particular, the fields that we've been using so far are:
:param <param>: to describe a parameter
:type <type>: to annotate the parameter type
:raises <exception>: to add exceptions that might be raised
:returns: to describe what is being returned
:rtype: to annotate the type of the return value

« Back to merge proposal