Merge lp://qastaging/~nataliabidart/ubuntuone-control-panel/default-folders into lp://qastaging/ubuntuone-control-panel

94 + with open(dirs_path) as f:
95 + while True:
96 + line = f.readline()

The file object you get back (f) is already iterable, so you could replace the "while True" with "for line in f"

Other than this it looks alright.

> 94 + with open(dirs_path) as f:
> 95 + while True:
> 96 + line = f.readline()
>
> The file object you get back (f) is already iterable, so you could replace the
> "while True" with "for line in f"
>
> Other than this it looks alright.

Fixed and pushed, great catch! thanks

86 + if dirs_path is None:
87 + dirs_path = os.path.join(user_home, '.config', 'user-dirs.dirs')

We probably want to use xdg_config_home from dirspec.basedir here, instead of assuming ~/.config/.

With the suggestion from Brian, you can also drop the if line is None: break check.

Do we need to check that translated names for directories get handled correctly as well here, and ensure that unicode handling doesn't break again? I don't see a test for that case in your tests.

Also, on Windows, for the special folders you check for, which aren't supported on XP for example, how is that error handled? I suppose there should be a test which causes and checks for the error when that happens, to ensure we handle it properly? The linked MSDN page seems to suggest that _MYVIDEO doesn't exist on 5.0 (XP) for example. It might also be worth noting in the comment that if/when we drop XP support, we probably want to switch to _MYDOCUMENTS instead of _PERSONAL for that folder; as the documentation seems to suggest we should do.

+1

+1

> 86 + if dirs_path is None:
> 87 + dirs_path = os.path.join(user_home, '.config', 'user-dirs.dirs')
>
> We probably want to use xdg_config_home from dirspec.basedir here, instead of
> assuming ~/.config/.

Added.

> With the suggestion from Brian, you can also drop the if line is None: break
> check.

Done already.

> Do we need to check that translated names for directories get handled
> correctly as well here, and ensure that unicode handling doesn't break again?
> I don't see a test for that case in your tests.

Added tests for this.

> Also, on Windows, for the special folders you check for, which aren't
> supported on XP for example, how is that error handled? I suppose there should
> be a test which causes and checks for the error when that happens, to ensure
> we handle it properly? The linked MSDN page seems to suggest that _MYVIDEO
> doesn't exist on 5.0 (XP) for example. It might also be worth noting in the
> comment that if/when we drop XP support, we probably want to switch to
> _MYDOCUMENTS instead of _PERSONAL for that folder; as the documentation seems
> to suggest we should do.

Good catch, made the code more robust and added tests for it.