ulc-devs team mailing list archive

[Frederik Elwert <frederik.elwert@xxxxxx>]

Hello Patrick,

Am Sonntag, den 27.11.2011, 00:16 +0100 schrieb Patrick Seemann:
> 
> I just managed to get the contacts lens daemon integrated into the
> quickly template.

Great! Thanks for the work.

> I'm not 100% sure if I did the right thing (it took me a while to
> figure out how this template works and what files are being called and
> initialized etc.) but it seems to be working.

They moved some bits since I started a quickly project the last time
(e.g., using __init__.py instead of bin/<appname> directly). But I think
you did it quite right, having some bits in __init__ and the main code
in lensdaemon.py makes sense to me.

> You should be able to grep the source, package it and install the
> lens:
> 
> 
> bzr branch lp:~patrickseemann/unity-lens-contacts/ulc-patrick
> cd ulc-patrick
> quickly package
> sudo dpkg -i ../unity-lens-contacts_0.1_all.deb

Yes, it worked smoothly.

> 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.

> 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).

> 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.

> 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.

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.

[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/

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.

And there is a nice set of plugins for gedit, "gedit-developer-plugins".
They include a python syntax checker that it very helpful. It discovered
that you use tabs in some placed, probably accidentally by pasting
snippets from examples.

If you could go over these minor issues, I’d be happy to merge the code.
This would give us a basically working lens to start from.

Thanks again for your work!

Regards
Frederik