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

[Dominik Moritz]

Hey Sean,

I'm strongly against docstrigs and :raises: everywhere as it gets out of sync and you can't rely on them. We would probably end up with something like the raises decorator in Java but without language support which will go wrong. Having said this, I am for :raises: declarations in the action functions docstrings as they help external developers a lot. However, they shouldn't list all exception types that can be raised from functions that are called because some exceptions will never occur. Consider an action function that calls another function with a static value. This function will never raise an exception. I don't see the point of adding an exception that never occurs to the docstring.  l also +1 your suggestion to always raise ckan.logic.Invalid from CKAN validator functions and no other exceptions. However, this should not mean a huge try...except bock around everything in the validator. Imho, if something goes horribly wrong (in other words an unexpected exception, not a validation error), it should be raised as such an exception. Wrapping, yes but within limits ;-)

We should probably differentiate between expected and unexpected exceptions. Expected exceptions (Validation errors, not authorized errors...) that are raised because strange input was provided should be documented. Unexpected exceptions such an a SocketError because the db is down should not be documented and treated as server errors. 

Cheers,
Dominik

On 13 Aug 2013, at 11:14, Sean Hammond <sean.hammond at okfn.org> wrote:

>> So, how would the documentation for a function available to an extension
>> would be? It would have a :raises: part for every exception that it raises,
>> plus every exception that the methods it calls raises as well?
> 
> Yep
> 
>> If so, how would we keep that list in sync?
> 
> Whenever you add or remove an exception from a method or function, you
> would have to grep the source code for anyone using that function and
> update their docstrings.
> 
> Also if you change a function so that it calls a new function (or
> potentially calls a function it was already calling, but with different
> params) then you need to check the function you're calling's docstring
> to check whether you need to update your own function's docstring.
> 
> I know it sounds pretty bad, but raising exceptions can make CKAN crash
> so I don't think I mind being quite strict about them.
> 
> I think Joe is onto something in his reply though, the way to keep the
> needed docstring-updating under control may be to reduce how often our
> functions throw exceptions and reduce the number of different types of
> exceptions that get thrown, including by catching exceptions and
> potentially wrapping them in other exceptions and re-raising, that sort
> of thing.
> 
> So for example all CKAN validator functions should raise
> ckan.logic.Invalid and never any other exception, auth functions should
> raise NotAuthorized, action functions may have a few different
> ckan.logic.* exceptions, but the general idea is it shouldn't just be
> that any function anywhere in CKAN might raise any random type of
> exception (whether the exception is listed in its docstring or not).
> 
> _______________________________________________
> 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/20130813/6f7e826e/attachment-0001.sig>