Code review comment for lp:~ian-clatworthy/bzr/411413-plugin-disable

2010/1/11 Ian Clatworthy <email address hidden>:
> Vincent Ladeuil wrote:
>
>> That's not in particular, windows is the only platform where we
>> have this problem.
>
> The issues I'm looking to solve impact all platforms IMO. Even on Linux,
> a user may wish to disable a plugin that's installed system-wide and
> they may not have permissions to touch the system paths where that
> plugin is installed.

This is definitely true. I commonly want to do it myself; obviously I
know how but moving the directory is a bit ugly. There are also bugs
where we want to ask people "can you reproduce this without bzr-svn"
and that is more complicated than it should be.

>> PATH is as magic as BZR_PLUGIN_PATH, I don't think it's relevant
>> here.
>
> I think you and I are trying to solve very different Use Cases. My
> drivers are:
>
> 1. More batteries should be included out-of-the-box in installers. Some
> users will want or need to disable some of those batteries and they'll
> most likely want to do it "forever". In some cases, plugins will be
> broken and they'll want to turn them off. Or they'll hit and bug and
> want to see whether having certain plugins installed fix the problem or
> otherwise. Others may simply prefer a smaller footprint, e.g. less
> entries in the list of commands when running qrun, less parts that might
> break or introduce errors, etc.
>
> 2. There should be a feature in qplugins for enabling/disabling plugins.
> That data ought to be permanently stored somewhere sensible like
> bazaar.conf. An environment variable just doesn't cut it.
>
> My focus is end-users, e.g. someone who clicks on a plugin in qplugins
> and is given the option to disable it or to select a particular instance
> of it found on the plugins path. Your focus seems to be test runners and
> plugin developers where the needs are far more short term.
>
>> You haven't even reply to the other conses....
>
> Fair enough:
>
> * "adds complexity" is a motherhood statement. Of course it does.
>  So do 99% of other new features.
>
> * "Developers can't give an explicit path". Agreed. I could
>  fix that. OTOH, current mechanisms (like symlinks) works fine
>  right now so it didn't feel worth it.

I suspect if you point to the parent directory, people will commonly
get it wrong and be confused. Well that will probably happen whatever
you do, but I think perhaps more so this way.

> * "Restriction to a single run". I like your idea of environment
>  variables for that case. You could just as nicely solve it by
>  using an environment variable pointing to a custom bazaar.conf, yes?
>  In that case, you could more easily test any custom settings in
>  bazaar.conf, not just custom plugin paths.
>
>> And you're already bringing back a feature which should clearly
>> be addressed by using BZR_PLUGIN_PATH.
>
> I'm sorry but that statement is simply not true. For your Use Cases,
> environment variables sound fine. For mine, they are the wrong choice.
>
> Having multiple mechanisms may indeed be the right answer here, given
> the different problems we're trying to address. Regardless, having zero
> mechanisms isn't the right number. :-)

It's reasonable to have this be persistent.

I strongly feel that eventually we want to move towards both
environment variables and files addressing the same space of
configuration options. At the moment things use one or the other
depending partly just on historical accidents. I don't want to block
Ian's change on getting that unification, but I'd like it done in such
a way that it can eventually mesh with it.

When that unification occurs, it addresses some of vincent's concerns:
setting for a single run, clashing with BZR_PLUGINS_PATH, etc.

To me having sections within bazaar.conf that act as namespaces for
the setting rather than (as in other configuration files) as a scope
in which the setting applies is a bit of a concerns. Saying 'bzr
-OPLUGINS:svn=' is a bit ugly (maybe only because of the uppercase
bit.)

--
Martin <http://launchpad.net/~mbp/>