Code review comment for lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-tracker

On Wed, Sep 7, 2011 at 11:09 PM, James Westby <email address hidden> wrote:
> Review: Approve
> Hi,
>
> Thanks for this, great to see it moving forward. Overall it looks good.
>
> 52      +def unicode_or_None(attr):
> 53      + if attr is None:
> 54      + return attr
> 55      + if isinstance(attr, unicode):
> 56      + return attr
> 57      + return attr.decode("utf-8")
>
> It would be great to not duplicate this further (apologies for the duplication
> that is currently there.)
>
> If it's used could you put it in a module or something?
>
> 39      +def data_error(spec_url, msg, warning=False):
>
> Deleting these sorts of unused things would be good. It would be better
> to do them in a better fashion if they are needed in this script.

Sure, I'll fix the above cleanups.

>
> 451     +def lanes(db):
> 452     + cur = db.cursor()
> 453     + cur.execute('SELECT name, lane_id from lane')
> 454     + return [(i[0], i[1]) for i in cur]
>
> I'd really like to re-use the ORM classes here, rather than hand-writing
> SQL. Perhaps we should be doing the work now to enable that?

That would be good. I spent a lot of time trying to find examples of
that, but now I understand why I didn't find it.

>
> The issue is that direct SQL access via the sqlite API interferes with
> Storm's caching. So we would need to change all the sql to do through
> Storm's SQL interface.
>
> It's a lot of code to change, but not difficult.
>
> Perhaps we could even add some sort of compatibility layer so that less
> code has to change?

If we need all explicit sql queries to go away before we can use the
ORM classes, then that might be a good idea. I've looked over the sql
queries and there are a lot of them. On the other hand, if we get rid
of all the hand-written sql in one go, then we won't be stuck
maintaining the left-overs. I'll try to get an idea of how much work
it would be.

>
> It doesn't have to stop this code from landing, but maybe it should
> be the next thing, as it will make later work easier.
>
> Also, do we want to add the code to actually generate the roadmap.html
> on status.linaro.org?

Right, I think I've found how to do that now.

>
> Thanks,
>
> James
>
> --
> https://code.launchpad.net/~mabac/launchpad-work-items-tracker/linaro-roadmap-tracker/+merge/74441
> You are the owner of lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-tracker.
>