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

[Sean Hammond]

I've added this to the Python coding standards, pull request:

https://github.com/okfn/ckan/pull/1211

On Wed, Aug 14, 2013 at 11:48:39AM +0200, Sean Hammond wrote:
> 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?