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

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

It makes sense, I actually prefer to use () than \ since for me it seems cleaner. Will get that fix.

>
> ===========================
> 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:"
>

On it! good catch!

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

Fixing

> ******
> + def add_child_watch(self, path):
>
> This new method looks fine to me.
> ******
> + """Add a new path to be watch.
>
> watch > watched

Fixing.

> ============
> os_helper.py
>
> native_listdir
>
> What is the purpose of having listdir and native_listdir if they are the same?

Really good question, and it is indeed a bug. os.listdir does return certain values, such as system folders, that we want to ignore. I should be using the wrapped method from os_helper.

Thx for the review!