Code review comment for lp:~abentley/bzr/appenddir

Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks for working on this, this is clearly a sore point in out config handling.

I've got a couple of questions about the implementation:

- are you aware of the work in progress documented in configuration.txt in the lp:~bzr/bzr/devnotes ? The interpolation part especially may provide a more scalable means to achieve the same result.

- why did you chose 'appendir' instead of say 'nick' ? I'm well aware that they may not match but the later would also work for looms,

27 STORE_LOCATION = POLICY_NONE
28 STORE_LOCATION_NORECURSE = POLICY_NORECURSE
29 STORE_LOCATION_APPENDPATH = POLICY_APPENDPATH
30 STORE_BRANCH = 3
31 STORE_GLOBAL = 4
32 +STORE_LOCATION_APPENDDIR = POLICY_APPENDDIR

The overlapping between STORE_* and POLICY_* is already a concern in the current implementation, could you think of a better way to articulate their relationship ?

This shows even more in:

51 - if store not in [STORE_LOCATION,
52 - STORE_LOCATION_NORECURSE,
53 - STORE_LOCATION_APPENDPATH]:
54 + if store not in _policy_name:

Err, wait, checking 'store' in '_policy_name' ? This at least requires a comment...

review: Needs Information

« Back to merge proposal