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

Wed Aug 14 10:18:28 UTC 2013

I think we should do this exactly as Sean wrote it down and add this to the python coding standards documentation [1].

In addition to what is written down we have to make sure that all exceptions that can be raised from a function are also in the toolkit [2]. At the moment these are:
* ObjectNotFound
* NotAuthorized
* UnknownConverter
* UnknownValidator
* ValidationError
* CkanVersionException

[1] http://docs.ckan.org/en/latest/python-coding-standards.html
[2] https://github.com/okfn/ckan/blob/master/ckan/plugins/toolkit.py

On 14 Aug 2013, at 11:48, Sean Hammond <sean.hammond at okfn.org> 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?
> 
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
> Unsubscribe: http://lists.okfn.org/mailman/options/ckan-dev

Dominik Moritz
CKAN developer  |  skype: d.moritz  |  @doobly_doo
The Open Knowledge Foundation
Empowering through Open Knowledge
http://okfn.org/  |  @okfn  |  http://ckan.org  |  @CKANproject

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20130814/252d2329/attachment-0001.sig>