ulc-devs team mailing list archive

[Patrick Seemann <patsee14@xxxxxxxxx>]

Hello,

On Sun, Nov 27, 2011 at 6:52 PM, Frederik Elwert <frederik.elwert@xxxxxx>wrote:


> > I added some lines to the __init__.py file in the unity_lens_contacts
> > folder to setup the dbus connection and finally run an instance of the
> > Daemon class, which is located in the lensdaemon.py file in the same
> > folder.
>
> I have to admin I do not yet fully understand what the code in
> __init__.py does and why it is needed in addition to the daemon code. I
> guess you try to get our bus name, so we can handle name conflicts
> before the lens init code hits them? GI based DBus code is still new to
> me, I used to use python-dbus in the past.
>
> I will try to add some comments to that code section. I have to admit
that I pretty much copied the code from the unity-lens-python example
on launchpad and since it works I didn't pay too much attention to it :)
I will try to find out more about GI based DBus connections - I'm
also new to it.


> > If you have any other ideas for better naming of the files
> > (lensdaemon.py), please feel free to tell me :) I wasn't sure if there
> > are already some naming conventions that we (should) follow?!
>
> I think we are relatively free, since these files live inside our own
> python package. If we want to follow the quickly conventions, we might
> think about renaming it to unity_lens_contactsdeamon.py (similar to the
> config file).
>

yes, I like that name better, too! thanks :)


> > I didn't add any useful functionality to the lens. I just wanted it to
> > work basically. So when you type some letters into the search box, it
> > just sorts them in categories (lower case, upper case).
>
> Yes, sure. Good to have a basic example of a lens that works. Once we
> have this in shape, I’ll ping the quickly devs and ask if they are
> interested in making a quickly template out of it.


that's a great idea!


> > Just to get clear on how the development workflow works:
> > - I branch the code from our main repository
> > - I make my changes
> > - I push everything to my own repository (does it matter what I call
> > this?, e.g. this time I named it simply ulc-patrick)
> > - someone (Frederik) merges my changes into our main repository
>
> Yes, correct. The name of your branch doesn’t really matter. I know two
> naming conventions: using your name, like you did it, for general work.
> And using feature names for specific work, like “new-icons”, or
> “bug-1234”. So I think it is fine to use you private branch for general
> work until the projects matures a bit, and then use feature branches for
> specific work.


The concept of using feature names for specific work sound good. I'll
try to do that when it seems fitting.


> Following Nigel’s advice[1], I’ll comment on your code a bit. In
> general, I like your code, so please don’t take these comments as
> criticism, but as hints from another pair of eyes looking at your code.
>

No! I very much appreciate your help!


> [1] http://nigelb.me/ubuntu/mozilla/2011/10/30/please-nitpick.html
>
>      * There seems to be a left-over test comment in
>        lib/unity-lens-contacts, that should be removed if not needed
>        any more.
>      * In unity_lens_contacts/__init__.py, I think using proper logging
>        for debug and error messages should be preferred over using
>        print statements. I try to avoid print at all in production code
>        (although I sometimes use it for quick debugging).
>      * You could be a bit more verbose in your comments. (I also tend
>        to write too few comments, but when developing together, it
>        really helps.) Specifically, I’d be happy about an explanation
>        what the DBus code does (not step by step, but the general
>        idea).
>      * We should follow PEP 8[2]. Unfortunately, many canonical devs
>        don’t follow it too closely, so tutorials and examples often
>        contain non-pythonic code. We should to better. :-) What I
>        noticed:
>              * Don’t use spaces before opening parentheses, e.g. "class
>                Daemon(object)" instead of "class Daemon (object)",
>                "cats.append(...)" instead of "cats.append (...)" etc.
>              * I think GI allows for a more pythonic way to create
>                class instances, so we should use "Unity.Category(...)"
>                instead of "Unity.Category.new(...)" if possible.
>              * Never use ; to end lines.
>              * Lines should not be longer than 79 characters.
>              * This is not a strict rule, but I prefer to use 'single
>                quotes' for string literals instead of "double quotes"
>                wherever possible. An exception are docstrings.
>      * You changed file mode to +x for many files. I don’t know if that
>        was on purpose? I think in general as few files as possible
>        should have +x. The one that does might be
>        lib/unity-lens-contacts, as it is our main executable.
>
> [2] http://www.python.org/dev/peps/pep-0008/
>
> wow this is my first code review ever I believe! Thank you very much :)
Very detailed! I guess since this is the first time for me working on
a project together with other people I never really payed attention to
any code conventions. I just did what looked right to me. I've always
tried to get at least some consistency within the code I was writing -
I always use 4 spaces of indention for example. I will try to stay
as close as possible to the pep-8 style guide for python in the future.

As for the files marked +x: I think this was because I branched the code
folder onto a ntfs-formatted partition (I use one because I still have a
windows
installation residing on my hard drive). I think ntfs doesn't get
permissions
right - at least that is what I'd read some time ago. I moved the folder to
my home partition and it seems to be working now. Thanks for the hint,
I didn't realize actually :)

By the way, bzr has some nice graphical tools to examine code changes,
> namely "bzr gdiff" and "bzr glog". glog also allows you to view the diff
> for specific revisions, much easier than fiddling with the command line
> syntax for this.
>

nice, just installed them - gdiff and glog are indeed very nice and easy to
use :)

Thank you again for your help, Frederik !
Regards,
-- 
Patrick Seemann

launchpad: ~patrickseemann
irc.freenode.net: patsee14