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

Revision history for this message
Natalia Bidart (nataliabidart) 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.

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.

« Back to merge proposal