Code review comment for lp://staging/~jelmer/bzr/cachedir

This looks like a good improvement overall, just are a few niggles in addition to the ones John has mentioned. It feels like cache_dir would be better suited to osutils than config, and possibly split into per-platform functions as there's not much sharing.

+ assert s is not None

There's a test in bt.test_source that fails if assert statements are used.

+ s = get_local_appdata_location()
...
+ if type(s) == str:

So, the bad api here isn't your fault, but adding type checks like this is the wrong way around it. The function needs fixing the return a consistent type.

+ cache_dir = s.encode(osutils._fs_enc)

This likely harmless in this specific case, but there's no guarantee a unicode path will be encodable to the filesystem encoding. Generally bzrlib treats paths as unicode, which is correct for windows and mac filesystems, having this api try and fit the path in a bytestring for the benefit of nix seems inconsistent.

+ if sys.platform in ("nt", "win32"):
...
+ else:

OSX is taking the XDG path on the `else:` here, which according to your tests is incorrect.

+ if type(cache_dir) == unicode:

Looks like an unreachable branch to me, all of the nix code deals in bytes only on Python 2.