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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-02-17 04:47 AM, Vincent Ladeuil wrote:
> Review: Needs Information
> 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 ?

No. I'm glad that serious re-thinking is going on. I would rather not
have to wait for a ground-up rewrite, unless it is immanent.

> The interpolation part especially may provide a more scalable means to achieve the same result.
>
> - why did you chose 'appendir' instead of say 'nick' ?

Initially, I did choose 'nick', but locations.conf is not branch-aware,
and in fact is sometimes used without a branch. So making it
branch-aware didn't seem like a good fit.

> I'm well aware that they may not match but the later would also work for looms,

Supporting looms would be nice, but holds little value for me. I
haven't used looms since I wrote bzr-pipeline.

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

It looks like inheritance to me-- All STORE values indicate which file
to store the value in, while the STORE_LOCATION ones also indicate a
policy. If I was doing a radical rewrite, I'd probably make them
classes with static/classmethods, e.g.

class ValueScope:

    @classmethod
    def store_value(cls, name, value):
        config = cls.get_config()
        cls._set_value(config, name, value)

    @staticmethod
    def _set_value(config, name, value)
        config.set_value(name, value)

class GlobalScope(ValueScope):

    @staticmethod
    def get_config():
        return GlobalConfig

class PlainLocationScope(ValueScope):

    @staticmethod
    def get_config():
        return LocationConfig

class AppendPathScope(PlainLocationScope):

    @staticmethod
    def _set_value(config, name, value):
        config.set_value(name, value, policy='appendpath')

A less-radical rewrite would be to make the STORE values tuples of
config, POLICY:

STORE_BRANCH = ('branch', POLICY_NONE)
STORE_LOCATION_APPENDPATH = ('location', POLICY_APPENDPATH)

No matter what, I don't think assigning integers to these things is
doing us any good. I'd love to see "STORE_BRANCH = 'branch'". That
would make debugging easier.

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

Yesterday, it looked like a nice way to avoid repeating ourselves.
Today, it looks a bit broken.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1dR4MACgkQ0F+nu1YWqI1IoACfcVJSN8zNbBnyjkzNeleExRrv
qh0An3yCEZ7KRXeOfFUR6Rya/TGMKexH
=lKhA
-----END PGP SIGNATURE-----