Code review comment for lp:~nataliabidart/ubuntuone-control-panel/default-folders

Revision history for this message
dobey (dobey) wrote :

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.

review: Needs Fixing

« Back to merge proposal