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.
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
