Code review comment for lp:~mandel/ubuntuone-client/add-virtual-watches

Revision history for this message
Brian Curtin (brian.curtin) wrote :

Here's a mini-review. I don't have my dev environment setup and don't know a lot of the context, plus I can't run it yet...but here's a few things I noticed.

######
test_filesystem_notifications.py

80 + self.mask = fs.FILE_NOTIFY_CHANGE_FILE_NAME | \
81 + fs.FILE_NOTIFY_CHANGE_DIR_NAME | \

Minor style thing, but I noticed in a big import (that was removed) that long lines were wrapped with parentheses instead of breaking with backslashes. Maybe do the same here? (Sorry, had to get my first nitpick cosmetic comment out of the way :)

===========================
filesystem_notifications.py

_get_partial_child_path_dir

It's about 2-3x faster to take a slice of the last character and compare it rather than using path.endswith. This may not be as clean as using endswith, but figured it may be worth mentioning. Something like "if not path[-1] == os.path.sep:"

Other than that, this method looks good.
******
+ self.log.warn('Adding watch to none existing path "%s"', path)

none existing > nonexistent
******
+ def add_child_watch(self, path):

This new method looks fine to me.
******
+ """Add a new path to be watch.

watch > watched
============
os_helper.py

native_listdir

What is the purpose of having listdir and native_listdir if they are the same?

« Back to merge proposal