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.

I.e. __init__ should accept file_name and get_filename and deprecate the later ?

>
> the class can then use it.

Conflicts with making a focused patch. I'll keep that for an updated submission focused on cleanup.

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

Yup, it's needed to build the lock directory (I'm sure you got that). The other classes postpone the get_filename() evaluation until it's needed, but given the functions used to provide the names, I don't think postponing is really required (some env variables are involved though, so there may be edge cases).

>
> 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.
>

I'll mention that. And yes, it's the real reason for requiring a larger lock scope.

> 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.

_can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

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

It's on a private method and used only by tests (and provide a poor way to build test configs anyway).

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

I'll restart from scratch then.

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

Most of them were required to address the flawed assumption that a config object (with content)
can be created without an actual file existing, I had to fix the failing tests.
The duplicated ensure_config_dir_exists() came in the way to.

And I mention that to explain that many cleanups were not for the sake of pure cleaning but driven by problems making the fix harder.

I'll look into an alternative solution to minimise the patch but that would likely end up in either weird code or intrusive changes, we'll see.