Patchwork [4,of,7,hglib] hglib: make util.cmdbuilder work with bytes (issue4520)

login
register
mail settings
Submitter Brett Cannon
Date March 26, 2015, 12:39 a.m.
Message ID <5efe91a4c2d49d74d110.1427330398@bcannon-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/8281/
State Accepted
Headers show

Comments

Brett Cannon - March 26, 2015, 12:39 a.m.
# HG changeset patch
# User Brett Cannon <brett@python.org>
# Date 1427329317 14400
#      Wed Mar 25 20:21:57 2015 -0400
# Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
# Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
hglib: make util.cmdbuilder work with bytes (issue4520)
Yuya Nishihara - March 26, 2015, 1:59 p.m.
On Wed, 25 Mar 2015 20:39:58 -0400, Brett Cannon wrote:
> # HG changeset patch
> # User Brett Cannon <brett@python.org>
> # Date 1427329317 14400
> #      Wed Mar 25 20:21:57 2015 -0400
> # Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
> # Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
> hglib: make util.cmdbuilder work with bytes (issue4520)
> 
> diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
> --- a/hglib/util.py	Wed Mar 25 20:19:09 2015 -0400
> +++ b/hglib/util.py	Wed Mar 25 20:21:57 2015 -0400
> @@ -103,7 +103,7 @@
>          if val is None:
>              continue
>  
> -        arg = arg.replace(b('_'), b('-'))
> +        arg = b(arg).replace(b('_'), b('-'))
>          if arg != b('-'):
>              if len(arg) == 1:
>                  arg = b('-') + arg
> @@ -115,10 +115,13 @@
>          elif isinstance(val, list):
>              for v in val:
>                  cmd.append(arg)
> -                cmd.append(str(v))
> +                cmd.append(v)

Doctest fails here.

% python -m doctest hglib/util.py
**********************************************************************
File "hglib/util.py", line 97, in util.cmdbuilder
Failed example:
    cmdbuilder('cmd', list=[1, 2])
Expected:
    ['cmd', '--list', '1', '--list', '2']
Got:
    ['cmd', '--list', 1, '--list', 2]
**********************************************************************

> +        elif isinstance(val, int):
> +            cmd.append(arg)
> +            cmd.append(strtobytes(val))
>          else:
>              cmd.append(arg)
> -            cmd.append(str(val))
> +            cmd.append(val)

Why not just replace str() by strtobytes() ?
Brett Cannon - March 26, 2015, 2:19 p.m.
On Thu, Mar 26, 2015 at 10:01 AM Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 25 Mar 2015 20:39:58 -0400, Brett Cannon wrote:
> > # HG changeset patch
> > # User Brett Cannon <brett@python.org>
> > # Date 1427329317 14400
> > #      Wed Mar 25 20:21:57 2015 -0400
> > # Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
> > # Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
> > hglib: make util.cmdbuilder work with bytes (issue4520)
> >
> > diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
> > --- a/hglib/util.py   Wed Mar 25 20:19:09 2015 -0400
> > +++ b/hglib/util.py   Wed Mar 25 20:21:57 2015 -0400
> > @@ -103,7 +103,7 @@
> >          if val is None:
> >              continue
> >
> > -        arg = arg.replace(b('_'), b('-'))
> > +        arg = b(arg).replace(b('_'), b('-'))
> >          if arg != b('-'):
> >              if len(arg) == 1:
> >                  arg = b('-') + arg
> > @@ -115,10 +115,13 @@
> >          elif isinstance(val, list):
> >              for v in val:
> >                  cmd.append(arg)
> > -                cmd.append(str(v))
> > +                cmd.append(v)
>
> Doctest fails here.
>
> % python -m doctest hglib/util.py
> **********************************************************************
> File "hglib/util.py", line 97, in util.cmdbuilder
> Failed example:
>     cmdbuilder('cmd', list=[1, 2])
> Expected:
>     ['cmd', '--list', '1', '--list', '2']
> Got:
>     ['cmd', '--list', 1, '--list', 2]
> **********************************************************************
>
>
Are doctests typically used?
http://mercurial.selenic.com/wiki/ContributingChanges doesn't mention them
and I would assume that if they were important they would be integrated
into the test.py script in hglib.

Regardless, how does this get fixed? Does this cause all of my patches to
get rejected and thus I have to go back and re-submit all of them? Does
Matt apply the other patches that don't break anything, skip the ones that
do, and then I re-submit just the ones that got rejected?


> > +        elif isinstance(val, int):
> > +            cmd.append(arg)
> > +            cmd.append(strtobytes(val))
> >          else:
> >              cmd.append(arg)
> > -            cmd.append(str(val))
> > +            cmd.append(val)
>
> Why not just replace str() by strtobytes() ?
>

It might work. strtobytes() post-dates all of these changes in my ported
local repo so I just didn't think about it. It's quite possible it will fix
this problem and negate the need to special-case int.
Idan Kamara - March 26, 2015, 2:29 p.m.
On Thu, Mar 26, 2015 at 7:19 AM, Brett Cannon <brett@python.org> wrote:
>
>
> On Thu, Mar 26, 2015 at 10:01 AM Yuya Nishihara <yuya@tcha.org> wrote:
>>
>> On Wed, 25 Mar 2015 20:39:58 -0400, Brett Cannon wrote:
>> > # HG changeset patch
>> > # User Brett Cannon <brett@python.org>
>> > # Date 1427329317 14400
>> > #      Wed Mar 25 20:21:57 2015 -0400
>> > # Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
>> > # Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
>> > hglib: make util.cmdbuilder work with bytes (issue4520)
>> >
>> > diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
>> > --- a/hglib/util.py   Wed Mar 25 20:19:09 2015 -0400
>> > +++ b/hglib/util.py   Wed Mar 25 20:21:57 2015 -0400
>> > @@ -103,7 +103,7 @@
>> >          if val is None:
>> >              continue
>> >
>> > -        arg = arg.replace(b('_'), b('-'))
>> > +        arg = b(arg).replace(b('_'), b('-'))
>> >          if arg != b('-'):
>> >              if len(arg) == 1:
>> >                  arg = b('-') + arg
>> > @@ -115,10 +115,13 @@
>> >          elif isinstance(val, list):
>> >              for v in val:
>> >                  cmd.append(arg)
>> > -                cmd.append(str(v))
>> > +                cmd.append(v)
>>
>> Doctest fails here.
>>
>> % python -m doctest hglib/util.py
>> **********************************************************************
>> File "hglib/util.py", line 97, in util.cmdbuilder
>> Failed example:
>>     cmdbuilder('cmd', list=[1, 2])
>> Expected:
>>     ['cmd', '--list', '1', '--list', '2']
>> Got:
>>     ['cmd', '--list', 1, '--list', 2]
>> **********************************************************************
>>
>
> Are doctests typically used?
> http://mercurial.selenic.com/wiki/ContributingChanges doesn't mention them
> and I would assume that if they were important they would be integrated into
> the test.py script in hglib.

Use 'make tests', it passes the flag needed to test.py to also run doctests.
Yuya Nishihara - March 26, 2015, 2:34 p.m.
On Thu, 26 Mar 2015 14:19:01 +0000, Brett Cannon wrote:
> On Thu, Mar 26, 2015 at 10:01 AM Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Wed, 25 Mar 2015 20:39:58 -0400, Brett Cannon wrote:
> > > # HG changeset patch
> > > # User Brett Cannon <brett@python.org>
> > > # Date 1427329317 14400
> > > #      Wed Mar 25 20:21:57 2015 -0400
> > > # Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
> > > # Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
> > > hglib: make util.cmdbuilder work with bytes (issue4520)
> > >
> > > diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
> > > --- a/hglib/util.py   Wed Mar 25 20:19:09 2015 -0400
> > > +++ b/hglib/util.py   Wed Mar 25 20:21:57 2015 -0400
> > > @@ -103,7 +103,7 @@
> > >          if val is None:
> > >              continue
> > >
> > > -        arg = arg.replace(b('_'), b('-'))
> > > +        arg = b(arg).replace(b('_'), b('-'))
> > >          if arg != b('-'):
> > >              if len(arg) == 1:
> > >                  arg = b('-') + arg
> > > @@ -115,10 +115,13 @@
> > >          elif isinstance(val, list):
> > >              for v in val:
> > >                  cmd.append(arg)
> > > -                cmd.append(str(v))
> > > +                cmd.append(v)
> >
> > Doctest fails here.
> >
> > % python -m doctest hglib/util.py
> > **********************************************************************
> > File "hglib/util.py", line 97, in util.cmdbuilder
> > Failed example:
> >     cmdbuilder('cmd', list=[1, 2])
> > Expected:
> >     ['cmd', '--list', '1', '--list', '2']
> > Got:
> >     ['cmd', '--list', 1, '--list', 2]
> > **********************************************************************
> >
> >
> Are doctests typically used?
> http://mercurial.selenic.com/wiki/ContributingChanges doesn't mention them
> and I would assume that if they were important they would be integrated
> into the test.py script in hglib.

They are integrated. See Makefile.

But, I have no idea how b'' can be handled in doctest on Python 3.

Regards,
Brett Cannon - March 26, 2015, 2:38 p.m.
On Thu, Mar 26, 2015 at 10:35 AM Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 26 Mar 2015 14:19:01 +0000, Brett Cannon wrote:
> > On Thu, Mar 26, 2015 at 10:01 AM Yuya Nishihara <yuya@tcha.org> wrote:
> >
> > > On Wed, 25 Mar 2015 20:39:58 -0400, Brett Cannon wrote:
> > > > # HG changeset patch
> > > > # User Brett Cannon <brett@python.org>
> > > > # Date 1427329317 14400
> > > > #      Wed Mar 25 20:21:57 2015 -0400
> > > > # Node ID 5efe91a4c2d49d74d1104edd50cd5a82003df4b6
> > > > # Parent  6d273d0a51aa24f9c837d67b656467f98f98786d
> > > > hglib: make util.cmdbuilder work with bytes (issue4520)
> > > >
> > > > diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
> > > > --- a/hglib/util.py   Wed Mar 25 20:19:09 2015 -0400
> > > > +++ b/hglib/util.py   Wed Mar 25 20:21:57 2015 -0400
> > > > @@ -103,7 +103,7 @@
> > > >          if val is None:
> > > >              continue
> > > >
> > > > -        arg = arg.replace(b('_'), b('-'))
> > > > +        arg = b(arg).replace(b('_'), b('-'))
> > > >          if arg != b('-'):
> > > >              if len(arg) == 1:
> > > >                  arg = b('-') + arg
> > > > @@ -115,10 +115,13 @@
> > > >          elif isinstance(val, list):
> > > >              for v in val:
> > > >                  cmd.append(arg)
> > > > -                cmd.append(str(v))
> > > > +                cmd.append(v)
> > >
> > > Doctest fails here.
> > >
> > > % python -m doctest hglib/util.py
> > > **********************************************************************
> > > File "hglib/util.py", line 97, in util.cmdbuilder
> > > Failed example:
> > >     cmdbuilder('cmd', list=[1, 2])
> > > Expected:
> > >     ['cmd', '--list', '1', '--list', '2']
> > > Got:
> > >     ['cmd', '--list', 1, '--list', 2]
> > > **********************************************************************
> > >
> > >
> > Are doctests typically used?
> > http://mercurial.selenic.com/wiki/ContributingChanges doesn't mention
> them
> > and I would assume that if they were important they would be integrated
> > into the test.py script in hglib.
>
> They are integrated. See Makefile.
>
> But, I have no idea how b'' can be handled in doctest on Python 3.
>

With compatibility with Python 2.5 and older it simply can't. You would
have to modify the doctest to either check different results based on
Python 2 or 3 or modify so that it basically becomes a unit test instead of
a doctest.

Patch

diff -r 6d273d0a51aa -r 5efe91a4c2d4 hglib/util.py
--- a/hglib/util.py	Wed Mar 25 20:19:09 2015 -0400
+++ b/hglib/util.py	Wed Mar 25 20:21:57 2015 -0400
@@ -103,7 +103,7 @@ 
         if val is None:
             continue
 
-        arg = arg.replace(b('_'), b('-'))
+        arg = b(arg).replace(b('_'), b('-'))
         if arg != b('-'):
             if len(arg) == 1:
                 arg = b('-') + arg
@@ -115,10 +115,13 @@ 
         elif isinstance(val, list):
             for v in val:
                 cmd.append(arg)
-                cmd.append(str(v))
+                cmd.append(v)
+        elif isinstance(val, int):
+            cmd.append(arg)
+            cmd.append(strtobytes(val))
         else:
             cmd.append(arg)
-            cmd.append(str(val))
+            cmd.append(val)
 
     for a in args:
         if a is not None: