Code review comment for lp:~nmb/bzr/xdgconfigdir

Revision history for this message
Andrew Bennetts (spiv) wrote :

It looks like you've addressed John's review nicely.

In doc/developers/xdg_config_spec.txt there's a typo:

> +%APPDATA%/Bazaar/2.0 and on Mac OS X, the directory is ~/.bazaaar. With the

You don't need quite so many “a”s in bazaar :)

> Note that the following descriptions do not apply
> +to Windows or Mac OS X which can and should use their own native configuration
> +locations.

True... although is ~/.bazaar the native location for Mac OS X? I'm not sure what
is expected on OS X, and improving the OS X experience is out of scope for this
patch, but this doc shouldn't be misleading about how good we are currently.

It would be nice to have tests for this, but I guess it's awkward to do. Hmm, I
suppose it's possible by overriding the environment variables for $HOME and/or
$XDG_CONFIG_HOME. TestConfigPath in bzrlib/tests/test_config.py is probably a
good starting point.

The only other comment I have is this is worth highlighting in What's New in 2.3
as well.

I think this should have some simple tests added before we merge it, so I'm voting
Needs Fixing, but basically this is looking pretty good.

review: Needs Fixing

« Back to merge proposal