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

>>>>> "Ian" == Ian Clatworthy <email address hidden> writes:

    >> Also, setting a config variable is not really better than
    Ian> issuing
    >> a 'mv plugin DISABLED/' to temporarily disable a plugin or
    >> issuing 'ln -s whatever-path ~/.bazaar/plugins/plugin' when
    >> installing a new plugin.

    Ian> I disagree here. For Windows users in particular,

That's not in particular, windows is the only platform where we
have this problem.

    Ian> the primary use case is disabling one or more plugins
    Ian> that came in the bundled installer. That's *much* nicer
    Ian> to do by editing bazaar.conf (either directly or via
    Ian> intelligence added to qplugins or qconfig) than asking
    Ian> users to hack zip files buried in system level
    Ian> directories.

Users shouldn't have to hack a zip file. I expressed concerns
about that weeks ago and I considered (and still consider) it a
bug in the windows installer.

Moreover it also means people installing bzr from source on
windows can't easily use the plugins embedded in the installer.

And I'm back to the idea that you're doubling file systems
operations without addressing the root cause (since epoch,
disabling a plugin has been done by moving it out of the way (the
trash being one possible target of the move :).

    Ian> Or asking them to set magic environment variable for
    Ian> that matter.

PATH is as magic as BZR_PLUGIN_PATH, I don't think it's relevant
here.

    >> So, regarding this patch:

    >> cons:
    >>
    >> - still rely on BZR_PLUGIN_PATH,

    Ian> I think it's fine to have that environment variable continue to
    Ian> be the path searched for plugins while also having configuration
    Ian> settings controlling disabling or otherwise. The question then
    Ian> becomes what are the sensible values for the configuration
    Ian> setting for a given plugin?

You haven't even reply to the other conses....

    Ian> One option is 'on' vs 'off'.

    Ian> Another is '' (disabled) vs 'use this one if multiple exist'.

And you're already bringing back a feature which should clearly
be addressed by using BZR_PLUGIN_PATH.

    Ian> I went with the latter because it was more powerful. I'm
    Ian> fine going with the first one (though it's a shame to
    Ian> dumb this feature down given your preferred alternative
    Ian> isn't likely to make the 2.1 IIUIC).

Feel free to enhance BZR_PLUGIN_PATH handling :) Shame or not
there are two features that has been defined and that will
address the same needs, you can either work on them or update the
docs to explain why bzr use two different mechanisms and cross
reference the two in the docs.

I still think using two different mechanisms will confuse users
more than it will help them.

Using config variables here sets a precedent, landing that patch
means we will have to continue supporting it even if
BZR_PLUGIN_PATH handling is enhanced: more work to come.