Code review comment for lp:~oubiwann/ubuntu-accomplishments-system/941255-unified-source-dir

Revision history for this message
Duncan McGreggor (oubiwann) wrote :

On Mon, Mar 5, 2012 at 1:13 AM, Jono Bacon <email address hidden> wrote:
> Review: Needs Fixing
>
> Thanks for your work on this, Duncan. :-)

Sure thing!

>
> This looks like quite a complex merge. Can you provide some more details of what does and what is involved?

You bet: at a high-level, this brings all the various libraries that
were independent under the same source code directory. This will allow
better refactoring in the future, where we can take advantage of
shared code and existing components from one part of the project in
another. The code base should actually shrink a bit, once we do that
(modularize).

At a lower level, here's what I did:

 * created a new package called "accomplishments" (after chatting with
you about names we could use)
 * moved trophyinfo_lib into accomplishments.gui.base, since those are
all base classes
 * moved trophyinfo into accomplishments.gui (these are the ones that
utilize a.g.base in the previous bullet)
 * moved daemon.libaccomplishments into accomplishments.daemon.dbusapi
 * moved deamon.libaccomplishments-daemon.daemon into accomplishments.deamon.app
 * moved other resources into media and data, as appropriate
 * as I touched parts of the code that needed PEP8 love or had
pylint/pyflakes, started addressing those too

> Also, given that we are going to wait to see what the Quickly team says about how dependencies are assessed, I would like to hold of merging changes that might break quickly.

This change shouldn't affect Quickly. I checked that "quickly run"
still works, and it does.

I'll poke around in quickly some more, and see if there are any hidden
impacts by having a changed source path (I did update ./bin/trophyinfo
to point to accomplishments.main and updated accomplishments.main to
point to all the new source stuff; I'm pretty sure that's why "quickly
run" is still happy).

« Back to merge proposal