[ckan-dev] All methods and functions in CKAN need docstrings with :raises:

Wed Aug 14 09:48:39 UTC 2013

Ok, I think you guys convinced my that adding :raises: to all docstrings
is a bad idea, as Dominik pointed out we'll never keep the docstrings in
sync so they won't be a reliable indicator anyway.

Talking to Dominik about this yesterday, what about this as an
alternative:

1. Functions exported for plugins, templates etc to use should have
docstrings with :raises: for every exception, because template and
plugin developers shouldn't have to read the CKAN core source code, but
internal CKAN functions shouldn't have these docstrings.

2. Whether it's an exported function or one only used internally, it
should only be raising exceptions from a small set of exception types
defined somewhere near the function.

For example logic/converters.py might have a set of one or more
exceptions defined at the top of the file, e.g.
ckan.logic.converters.Invalid, and all the converter functions are only
allowed to raise those types of exceptions. (Or maybe the exceptions are
defined in logic/__init__.py and shared between all modules in logic,
but they shouldn't be too far from the code.)

If any function raises an exception type from outside of its "set"
(which currently some of the functions for plugins and templates do)
that should be considered a bug and refactored, rather than documented
in docstrings.

3. To make up for not having docstrings with :raises: everywhere, we say
that a dev should be able to scan the source code of a function for the
keyword `raise` and see all the exceptions that a function raises. *You
shouldn't have to look at the code of functions that a function calls,
to find out about exceptions that the first function might raise*.

This means that when you're writing a function and you call some other
function, you need to check whether that other function raises any
exceptions and catch them, then either:

- handle the exception, or

- if it's an exception from the set of exception types your functiopn
  can raise then you can simply re-raise it (in which case the only
  point of catching and re-raising it is to point out to anyone reading
  your function's source code that it might raise this)

- or if it's not one of the exception types you're allowed to raise, wrap it in
  an exception you can raise and raise that

What do people think of this?