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

Wed Aug 14 11:36:15 UTC 2013

Hi Sean,

Sounds reasonable to me, we shouldn't be throwing random exceptions.

John

On 14 August 2013 12:18, Dominik Moritz <dominik.moritz at okfn.org> wrote:

> 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
>
>
> _______________________________________________
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20130814/58020254/attachment-0001.html>