Code review comment for lp:~vila/bzr/948339-config-caching

Revision history for this message
Martin Packman (gz) wrote :

Summarising from discussion on IRC yesterday,

The lock count change doesn't seem to be directly asserted by test_set_delays_write. Checking now though the test does in fact fail without the fix, as does test_set_from_config_stack_get_from_config in a similar manner, so that's fine.

Flipping the expectedFailure assertions where the behaviour isn't worth fixing is a little odd, though the added reasoning is nice. Ideally, it would live not here but somewhere that people trying to use config and running into problems might actually find.

The added test_mutable_section_shared is nice and easy to follow, so even though I find the list->dict fixes slightly mysterious without working out more about the config details, I'm happy with the change.

review: Approve

« Back to merge proposal