Code review comment for lp:~vila/bzr/525571-lock-bazaar-conf-files

16:24 < lifeless> vila: --config please, not --conf
16:25 < lifeless> --conf is a strange abbreviation
16:25 < lifeless> c = config.LockableConfig(lambda : location)
16:25 < lifeless> reads really strangely
16:25 < lifeless> perhaps the LockableConfig constructor should be different
16:27 < vila> lifeless: I don't want to break the chain of constructors, I can add a def get_filename if you prefer

When a class provokes ugly code in users, the class is wrong. Perhaps the class could take two keyword constructors, and then only the new code in builtins uses a keyword, and uses the second one to supply a name.

the class can then use it.

The ironic thing here is the first thing the class does is make the filename by calling the constructor.

Please note in the (otherwise excellent) docstring that the reason callers need to lock_write, rather than the innermost writer function, is to ensure that the file is both *read* and *written* in the same lock.

Perhaps that would be best tracked - when doing a read, if the object is not locked, set self._can_write = False, and if it is, set it to True. Then check that as well as whether it is locked, in _write_config.

The file=None to infile=None change seems to just generate spurious noise, and is not done in a backward compatible way.

We are likely going to want to backport this for launchpad, so it really should be a focused patch.

Likewise the test changes that are not related to locking consistency.

Please roll those things back, and I think we'll have a much more focused patch that we can confidently get LP to cherrypick.