[Merge] lp:~jamesodhunt/upstart/python-upstart-module into lp:upstart

[Barry Warsaw]

You can get rid of the import string; pyflakes tells me that it's not used.  Other pyflakes warnings include:

In Job.instances(), the 'remote_object' local variable is not used.  However, I see in the very next line that you're passing in self.remote_object as the first argument to dbus.Interface().  Perhaps that should have just been 'remote_object'?

In SessionInit.__init__(), the 'watch' variable is assigned to but never used.  Maybe just get rid of the assignment?

In TestUpstart.test_session_init_reexec(), the fd variable is assigned to but never used.  I'm not sure what the intent is for this one.

Other comments:

* wait_for_file() seems racy.  Also, it's not doing exactly what it claims because a time.sleep(1) because that call can sleep more or less than a second based on various factors.  When I want to do more accurate time based waits, I use a loop like this:

from datetime import datetime, timedelta
...

    until = datetime.now() + timedelta(5)
    while datetime.now() < until:
        if do_check():
            return True
        time.sleep(0.1)
    return False

You can of course tweak the details, but the basic idea is that you set up a point in the future which you'll check until and then the loop exit condition looks to see if *now* is past that future time.

Although it doesn't hurt, it's mildly unpythonic to use parens with an empty base class list, e.g. "class Upstart()".  We usually omit the parens (I think the empty base class syntax was actually illegal in Python 2 ;).

PEP 8 wants lines less than 79 characters wide.  You have a few lines that are too long.  Suggestions for wrapping include, instead of:

        self.remote_object = self.connection.get_object(object_path=OBJECT_PATH)

use

        self.remote_object = self.connection.get_object(
            object_path=OBJECT_PATH)

and instead of

        raise UpstartException('Failed to reconnect to Upstart after %d seconds' % timeout)

use

        raise UpstartException(
            'Failed to reconnect to Upstart after %d seconds' % timeout)

Remember that no backslash is necessary inside parens/braces/brackets!

In polling_connect(), you have a bare-except.  Generally you should avoid such bare-excepts because they can and do mask all sorts of exceptions you aren't expecting.  Better to narrow that down to exactly the set of exceptions you expect and want to ignore from self.connect().  (Aside: Python 3.4 will have a very cool contextlib.ignored() helper to ignore specific sets of exceptions.  Doesn't help you now though ;).

In general, current best practices these days is to avoid double-leading-underscore attributes such as __timeout_cb() and __idle_create_job_cb().  Python does name mangling on double-leading-underscore names, which can make debugging and introspection more difficult.  DLU attributes should only be used where potential namespace collisions with subclasses are a possibility (i.e. as part of a "protected" API for subclasses).  In most of these cases, a single leading underscore is enough to say "hey, this is my private stuff, don't use it!" without the inconvenience of name mangling.

In __idle_create_job_cb(), since at least two arguments are required (in addition to self), you can define the method like so:

def __idle_create_job_cb(self, name, body, *args)

and just ignore 'args'.  Then you don't need to explicitly unpack args[0] and args[1].

In __job_added_cb(), you don't need parens around the arguments to assert.  It's a statement, not a function in Python.  It's also usually a good idea to add a more meaningful error message as the second expression for assert, e.g.

    assert self.timeout_source, 'Why is there no timeout source? {}'.format(self.timeout_source)

Asserts also get compiled away when __debug__ is False, or when python3 -OO is used.

.get_test_dir() isn't used anywhere, and since it's not a property, it doesn't really save you anything over just accessing self.test_dir directly.

In create_dirs(), it's usually better (and less racy!) to try to create the directory and catch the FileExistsError that can result if the directory already exists.

In destroy(), os.rmdir() will fail if the directory is not empty.  Use shutil.rmtree() to really zap the whole directory and all its contents.

In emit(), don't set the env=[] parameter this way.  This is a subtle gotcha in Python - the parameter's default value is set when the method is defined, *not* when it's called.  If for example, dbus.Array() modified 'env', that would affect all future callers of emit.  Try running this code to see it in action:

def foo(env=[]):
    print(env)
    env.append('yes ')

foo()
foo()
foo()


:).  Much better to:

def emit(self, event, env=None, wait=True):
    if env is None:
        env = []

to ensure that env always gets a fresh empty list by default.

In version(), don't compare "raw == True".  We almost never explicitly compare against True and False, and never use == to compare against singletons.  "if raw:" is usually good enough.  Also in this method, you probably don't need the extra set of parens around the return value.  E.g. this should do the trick:

return version_string.split()[2].strip(')')

Yikes!  You have mixed tabs and spaces in reexec().  Python will barf on that.  See lines 300-301.

You might want to think about using ''.format() instead of % interpolation.  I think it leads to more readable code, and it definitely allows you to avoid backslashes, which seem ugly to me ;).  E.g. instead of

self.job_object_path = '%s/%s/%s' % \
    (OBJECT_PATH, 'jobs', dbus_encode(job_path))

use

self.job_object_path = '{}/{}/{}'.format(OBJECT_PATH, 'jobs', dbus_encode(job_path))

(and actually, in the first case, if you move the open paren to the end of the first line, you won't need the backslash anyway.  But still, I love .format() :)

On lines 355-357, better to indent the arguments to self.connection.add_signal_receiver() by 4 spaces.

Line 364 has another comparison using equality to a boolean.  Better to just use "if not job.seen:".

Line 422 could use this instead: "self.conffile = os.path.join(self.job_dir, self.name + '.conf')"

There are a few things about the code that writes to conffile that bug me.  The first is that you should use a with-statement to ensure that the file gets closed when you're done with it.  Second, by default open(..., 'w') will open the file in text mode, but you're not defining an encoding, and Python will use a platform-dependent value by default.  This will probably work fine until it doesn't and then it'll be a PITA to debug. ;).  Best to be explicit.  E.g. if you want the conf file to be a text file (i.e. instead of bytes), then open it with a specific encoding, probably utf-8.  Lastly, since body can be a list of strings or a string, why not splitlines() the string value and then treat them the same.  All together, I'd probably rewrite this code like so:

if isinstance(body, str):
    # Assume body cannot be a bytes object.
    body = body.splitlines()
with open(self.conffile, 'w', encoding='utf-8') as fh:
    for line in body:
        print(line.strip(), file=fh)
    print(file=fh)

This is untested, but I think you get the idea!

Maybe clean up the long lines and backslashes on lines 436-440.

The bare-except in destroy() also bugs me.  It also makes no sense to use a finally: statement with a bare-except, since all exceptions will get caught (and in this case ignored) so the finally is kind of useless.

def start() has another mutable function argument.

In _get_instance(), you know that with the default arguments, you could end up with an object_path like 'foo/None' right?

The backslash in instance_object_paths() is unnecessary.  You should also indent the 'for' to the character after the open brace.

In pids(), name will be '_' under more conditions of falseness that perhaps you expect.  Probably better to write this as a ternary statement, e.g.:

    name = ('_' if name is None else name)

(the parens are not strictly necessary here.  They are my one bow to readability, unlike the unnecessary parens for the assert statement :)

In method running(), you actualy don't need the ternary statement!  "return len(self.pids(name)) > 0" will do the trick. :)

In logfile_name(), probably better to use "logfile = os.path.join(self.upstart.log_dir, filename)"

Okay, that's 50% of the file.  To keep these manageable, I'll do the rest of the review in a second comment...
-- 
https://code.launchpad.net/~jamesodhunt/upstart/python-upstart-module/+merge/157549
Your team Upstart Reviewers is requested to review the proposed merge of lp:~jamesodhunt/upstart/python-upstart-module into lp:upstart.