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

Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

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

I disagree here. For Windows users in particular, the primary use case is disabling one or more plugins that came in the bundled installer. That's *much* nicer to do by editing bazaar.conf (either directly or via intelligence added to qplugins or qconfig) than asking users to hack zip files buried in system level directories. Or asking them to set magic environment variable for that matter.

> So, regarding this patch:

> cons:
>
> - still rely on BZR_PLUGIN_PATH,

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

One option is 'on' vs 'off'.

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

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

« Back to merge proposal