[ckan-dev] followers and thoughts

[Toby Dacre]

On 1 June 2012 10:28, Sean Hammond <sean.hammond at okfn.org> wrote:

> > I've had a look at the followers stuff following a bug I found, and
> looking
> > to add it to the demo site.
> >
> > I'd like to move the logic from the controllers into a helper function.
> > The reason for this is that it means that we don't need to do the work
> > finding followers if the feature isn't being used in a particular ckan
> > instance.  It also decouples it from the controllers so it would be
> > available even with a customised controller.
> >
> > essentially I'd remove the follower stuff from the controllers and get
> the
> > helper function to do the work.  All the logic work would remain the
> same.
> > It isn't yet in a released version of ckan so no-one should be affected.
> >
> > so we'd end up with something close to h.follow_button('dataset',
> pkg_id),
> > h.follow_numbers('user', user_name)
>
> Yes! This seems like a good idea to me. Just so everyone's on the same
> page, I think the only logic on the controllers you're talking about is
> that they call *_follower_list() and *_follower_count() action functions
> and save the results in the context for the templates to retrieve. This
> was one of the biggest pain points implementing follower because I had
> to do this in each controller method that needs it: in read(), authz(),
> resource_read() and history() in the package controller, a few places in
> the user controller, and also in the new related controller. Of course
> you can make a private helper method in the controller class to do it
> but you still have to make sure each controller method that needs it
> calls that helper method. So having to write it just once in a helper
> function sounds much better, and will also mean that the action
> functions aren't called unless the template actually uses them.
>
> A couple of thoughts I had about it, what if the same template calls the
> same helper multiple times with the same args? Would make unnecessary
> database accesses. The helper function could do some caching perhaps,
> e.g. against the pylons context, to avoid this. Not likely to happen
> with follower or activity streams anyway.
>

I think that caching is helpful as you say for followers it's not an issue
but for some things it will be.  I may not bother doing this yet.  I think
we also need to think about name spaces for c to prevent clashes maybe have
a c.__ckan_cache__ or something.

>
> Would the helper function be much more than a boilerplate wrapper for
> the action function? What about just calling the action API from to
> templates? I guess the helper function will also handle wrapping the API
> output in the <button> HTML, etc.
>

Yes it needs the wrapping.  I think I'll do this via a snippet so that the
actual html is easy to customise for a ckan instance

>
> > part of the reason for this is situations like the ckan index page
> building
> > the recent activity stream but not then displaying it. So all ckan
> > instances end up taking a hit because of the needs of a few instances.
> I'd
> > like to slowly move such things out of the controllers where
> > possible/appropriate.
>
> Yes, this seems like a good idea to me, lets us throw in new 'extra'
> features like follower and activity streams without slowing down page
> loads if you're not using them.
>
> This is the kind of thing that I think we should be writing about in the
> coding standards. Sure we should link to general stuff like PEP 8 and
> the Google Python Style Guide etc., but even more useful will be
> specific things about the right way to do things in CKAN.
>

I think this is a good idea.  We need to move our coding standards on from
4 spaces to these are the design pattens that we try to use.  pep8 is
easier to explain though.

Thanks
Toby

>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20120601/e7920bce/attachment-0001.html>