[MERGE REVIEW] Remove ConfigObj validation

Martin Pool mbp at canonical.com
Mon Jun 12 00:18:43 BST 2006


On 11 Jun 2006, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> I've submitted a version that changes the name, adds a docstring, and
> uses sys.executable.

Thanks.

> I haven't changed the retcode handling:
> Currently, run_bzr_subprocess is variadic.  You can call it like this:
> run_bzr_subprocess('merge', 'upstream', retcode=3)
> 
> If retcode becomes an explicit argument, then it becomes positional
> (even if a default is supplied).  So it gets the value 'merge', which is
> not desirable.

Ah, that's right.

> In order to make retcode explicit, we'd have to turn *args into a normal
> argument, and call it like this:
> run_bzr_subprocess(['merge', 'upstream'], retcode=3)
> 
> There's still the matter of shlex.  I'd prefer to keep it in, but I can
> take it out if people insist.

It's not so much shlex but rather the general principle that function
arguments should have one meaning.  This has caused fragility in the
past.  None of the existing uses of run_bzr_subprocess depend on shlex
so please take it out.

If there's a case where it's strongly needed I'd rather add a different
method that does only shlex and then calls run_bzr_subprocess

-- 
Martin




More information about the bazaar mailing list