Patchwork [6,of,7] py3: use try/except to check for basestring

login
register
mail settings
Submitter Pulkit Goyal
Date Nov. 2, 2016, 10:23 p.m.
Message ID <aef03902a5c1a13e9775.1478125391@pulkit-goyal>
Download mbox | patch
Permalink /patch/17303/
State Accepted
Headers show

Comments

Pulkit Goyal - Nov. 2, 2016, 10:23 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1478121825 -19800
#      Thu Nov 03 02:53:45 2016 +0530
# Node ID aef03902a5c1a13e9775059a5efdeb2466399ada
# Parent  9e259e7b59b6358eb842eabbc99f4c18a4cc5009
py3: use try/except to check for basestring

The term basestring don't exist in Python 3.5 and throws a NameError.
It used to refer to unicodes in Python 2. Used try/expect to handle this.
Yuya Nishihara - Nov. 4, 2016, 3:51 a.m.
On Thu, 03 Nov 2016 03:53:11 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1478121825 -19800
> #      Thu Nov 03 02:53:45 2016 +0530
> # Node ID aef03902a5c1a13e9775059a5efdeb2466399ada
> # Parent  9e259e7b59b6358eb842eabbc99f4c18a4cc5009
> py3: use try/except to check for basestring
> 
> The term basestring don't exist in Python 3.5 and throws a NameError.
> It used to refer to unicodes in Python 2. Used try/expect to handle this.
> 
> diff -r 9e259e7b59b6 -r aef03902a5c1 mercurial/ui.py
> --- a/mercurial/ui.py	Thu Nov 03 02:27:46 2016 +0530
> +++ b/mercurial/ui.py	Thu Nov 03 02:53:45 2016 +0530
> @@ -520,7 +520,12 @@
>          result = self.config(section, name, untrusted=untrusted)
>          if result is None:
>              result = default or []
> -        if isinstance(result, basestring):
> +        checkunicode = False
> +        try:
> +            checkunicode = isinstance(result, basestring)
> +        except NameError:
> +            checkunicode = isinstance(result, str)
> +        if checkunicode:
>              result = _configlist(result.lstrip(' ,\n'))

Given "result" is a source variable of a list to be returned, it shouldn't
be a unicode. So we can simply use bytes instead of basestring here.
Pulkit Goyal - Nov. 6, 2016, 6:45 p.m.
This https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089099.html
is a better version of what I want to do, since this didn't went
through I will be using this.

On Fri, Nov 4, 2016 at 9:21 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Thu, 03 Nov 2016 03:53:11 +0530, Pulkit Goyal wrote:
>> # HG changeset patch
>> # User Pulkit Goyal <7895pulkit@gmail.com>
>> # Date 1478121825 -19800
>> #      Thu Nov 03 02:53:45 2016 +0530
>> # Node ID aef03902a5c1a13e9775059a5efdeb2466399ada
>> # Parent  9e259e7b59b6358eb842eabbc99f4c18a4cc5009
>> py3: use try/except to check for basestring
>>
>> The term basestring don't exist in Python 3.5 and throws a NameError.
>> It used to refer to unicodes in Python 2. Used try/expect to handle this.
>>
>> diff -r 9e259e7b59b6 -r aef03902a5c1 mercurial/ui.py
>> --- a/mercurial/ui.py Thu Nov 03 02:27:46 2016 +0530
>> +++ b/mercurial/ui.py Thu Nov 03 02:53:45 2016 +0530
>> @@ -520,7 +520,12 @@
>>          result = self.config(section, name, untrusted=untrusted)
>>          if result is None:
>>              result = default or []
>> -        if isinstance(result, basestring):
>> +        checkunicode = False
>> +        try:
>> +            checkunicode = isinstance(result, basestring)
>> +        except NameError:
>> +            checkunicode = isinstance(result, str)
>> +        if checkunicode:
>>              result = _configlist(result.lstrip(' ,\n'))
>
> Given "result" is a source variable of a list to be returned, it shouldn't
> be a unicode. So we can simply use bytes instead of basestring here.
Yuya Nishihara - Nov. 7, 2016, 12:25 p.m.
On Mon, 7 Nov 2016 00:15:21 +0530, Pulkit Goyal wrote:
> This https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089099.html
> is a better version of what I want to do, since this didn't went
> through I will be using this.

I'm okay with that pycompat.basestring stuff, but I'm pretty sure most of
our basestring uses are moot since we avoid using unicodes except for very
specific string manipulations.

> >> @@ -520,7 +520,12 @@
> >>          result = self.config(section, name, untrusted=untrusted)
> >>          if result is None:
> >>              result = default or []
> >> -        if isinstance(result, basestring):
> >> +        checkunicode = False
> >> +        try:
> >> +            checkunicode = isinstance(result, basestring)
> >> +        except NameError:
> >> +            checkunicode = isinstance(result, str)
> >> +        if checkunicode:
> >>              result = _configlist(result.lstrip(' ,\n'))

And with this change, ui.configlist() would look as if it supports unicodes,
which seems confusing.
Pulkit Goyal - Nov. 7, 2016, 3:07 p.m.
On Mon, Nov 7, 2016 at 5:55 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Mon, 7 Nov 2016 00:15:21 +0530, Pulkit Goyal wrote:
>> This https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089099.html
>> is a better version of what I want to do, since this didn't went
>> through I will be using this.
>
> I'm okay with that pycompat.basestring stuff, but I'm pretty sure most of
> our basestring uses are moot since we avoid using unicodes except for very
> specific string manipulations.
>
>> >> @@ -520,7 +520,12 @@
>> >>          result = self.config(section, name, untrusted=untrusted)
>> >>          if result is None:
>> >>              result = default or []
>> >> -        if isinstance(result, basestring):
>> >> +        checkunicode = False
>> >> +        try:
>> >> +            checkunicode = isinstance(result, basestring)
>> >> +        except NameError:
>> >> +            checkunicode = isinstance(result, str)
>> >> +        if checkunicode:
>> >>              result = _configlist(result.lstrip(' ,\n'))
>
> And with this change, ui.configlist() would look as if it supports unicodes,
> which seems confusing.
Can you cherry pick some commits from that series?
Yuya Nishihara - Nov. 8, 2016, 2:16 p.m.
On Mon, 7 Nov 2016 20:37:14 +0530, Pulkit Goyal wrote:
> On Mon, Nov 7, 2016 at 5:55 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Mon, 7 Nov 2016 00:15:21 +0530, Pulkit Goyal wrote:
> >> This https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089099.html
> >> is a better version of what I want to do, since this didn't went
> >> through I will be using this.
> >
> > I'm okay with that pycompat.basestring stuff, but I'm pretty sure most of
> > our basestring uses are moot since we avoid using unicodes except for very
> > specific string manipulations.
> >
> >> >> @@ -520,7 +520,12 @@
> >> >>          result = self.config(section, name, untrusted=untrusted)
> >> >>          if result is None:
> >> >>              result = default or []
> >> >> -        if isinstance(result, basestring):
> >> >> +        checkunicode = False
> >> >> +        try:
> >> >> +            checkunicode = isinstance(result, basestring)
> >> >> +        except NameError:
> >> >> +            checkunicode = isinstance(result, str)
> >> >> +        if checkunicode:
> >> >>              result = _configlist(result.lstrip(' ,\n'))
> >
> > And with this change, ui.configlist() would look as if it supports unicodes,
> > which seems confusing.
> Can you cherry pick some commits from that series?

Can you pick out good ones?

FYI, the basestring patch had a problem, which didn't work on Python 2.
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089160.html
Pulkit Goyal - Nov. 8, 2016, 2:35 p.m.
> Can you pick out good ones?
These ones!
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089096.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089097.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089098.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089100.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089103.html
- You can drop u'' there in flight
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089102.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089104.html

They all look good and are a part of fixing things while `hg version`
on Python 3. Still if you find one wrong, drop that so that I can
follow up with a better patch.

>
> FYI, the basestring patch had a problem, which didn't work on Python 2.
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089160.html

Okay I will send a better patch which will fix that.
Yuya Nishihara - Nov. 9, 2016, 12:39 p.m.
On Tue, 8 Nov 2016 20:05:51 +0530, Pulkit Goyal wrote:
> > Can you pick out good ones?
> These ones!

Pushed some of them, thanks.

> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089096.html

Already queued.

> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089097.html
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089098.html
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089100.html

Queued these.

> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089103.html
> - You can drop u'' there in flight

Dropped per discussion.

> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089102.html

Added comment.

> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089104.html

Syntax error on Python 2.
Pulkit Goyal - Dec. 1, 2016, 11:02 p.m.
There is something strange. One of the patch you pushed and replied to
the mail is not the same.

On Wed, Nov 9, 2016 at 6:09 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Tue, 8 Nov 2016 20:05:51 +0530, Pulkit Goyal wrote:
>> > Can you pick out good ones?
>> These ones!
>
> Pushed some of them, thanks.
>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089096.html
>
> Already queued.
>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089097.html
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089098.html
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089100.html
>
> Queued these.
>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089103.html
>> - You can drop u'' there in flight
>
> Dropped per discussion.

I can't see the u'' dropped.
https://www.mercurial-scm.org/repo/hg-committed/rev/9df29b7c62cf
>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089102.html
>
> Added comment.

I don't know what you mean here as this is not pushed.
>
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089104.html
>
> Syntax error on Python 2.

But you pushed this one.
https://www.mercurial-scm.org/repo/hg-committed/rev/954002426f78.

I am unsure of whether the reply contained wrong links or you pushed
wrong ones. Let me know so that I can follow up accordingly.
Yuya Nishihara - Dec. 2, 2016, 1:55 p.m.
On Fri, 2 Dec 2016 04:32:47 +0530, Pulkit Goyal wrote:
> There is something strange. One of the patch you pushed and replied to
> the mail is not the same.

I've queued new versions of these patches.

> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089102.html
> >
> > Added comment.

Er, I don't remember. I guess I would wanted to tell you that I wrote some
comments to that patch.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-November/090218.html

Patch

diff -r 9e259e7b59b6 -r aef03902a5c1 mercurial/ui.py
--- a/mercurial/ui.py	Thu Nov 03 02:27:46 2016 +0530
+++ b/mercurial/ui.py	Thu Nov 03 02:53:45 2016 +0530
@@ -520,7 +520,12 @@ 
         result = self.config(section, name, untrusted=untrusted)
         if result is None:
             result = default or []
-        if isinstance(result, basestring):
+        checkunicode = False
+        try:
+            checkunicode = isinstance(result, basestring)
+        except NameError:
+            checkunicode = isinstance(result, str)
+        if checkunicode:
             result = _configlist(result.lstrip(' ,\n'))
             if result is None:
                 result = default or []