| Message ID | 1445030131-24566-3-git-send-email-vapier@gentoo.org |
|---|---|
| State | Accepted |
| Headers | show |
> From: Mike Frysinger <vapier@chromium.org> > > This fixes a few random issues to make the script work at least somewhat > under python 3: > - set the default encoding to utf-8 > - handle xmlrpclib/xmlrpc.client module renames > - handle ConfigParser/configparser module renames > - add a unicode() stub for python 3 > - fix old style class definition w/Filter > - use list comprehension instead of map() > - drop the unused version= keyword w/argparse > > The code still runs under python 2 the same as before, and now works for > the most part under python 3 -- the handling of encoded content still needs > some work, but that'll require more surgery, and is best left to another > commit after this. You're right - it needs a little more work but it's most of the way there. Do you plan to do this? I was looking at making use of Django's provided 'six' library to solve this issue for the server - we could use it here also. Regardless, for this patch Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> > Signed-off-by: Mike Frysinger <vapier@chromium.org> > --- > patchwork/bin/pwclient | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient > index d096f83..d2b95e5 100755 > --- a/patchwork/bin/pwclient > +++ b/patchwork/bin/pwclient > @@ -1,4 +1,5 @@ > #!/usr/bin/env python > +# -*- coding: utf-8 -*- > # > # Patchwork command line client > # Copyright (C) 2008 Nate Case <ncase@xes-inc.com> > @@ -23,16 +24,31 @@ from __future__ import print_function > > import os > import sys > -import xmlrpclib > +try: > + import xmlrpclib > +except ImportError: > + # Python 3 has merged/renamed things. > + import xmlrpc.client as xmlrpclib > import argparse > import string > import tempfile > import subprocess > import base64 > -import ConfigParser > +try: > + import ConfigParser > +except ImportError: > + # Python 3 has renamed things. > + import configparser as ConfigParser > import shutil > import re > > +# Add a shim for Python 2's unicode() helper. > +try: > + unicode > +except NameError: > + # Python 3 does everything by unicode now. > + unicode = str > + > # Default Patchwork remote XML-RPC server URL > # This script will check the PW_XMLRPC_URL environment variable > # for the URL to access. If that is unspecified, it will fallback to > @@ -40,7 +56,7 @@ import re > DEFAULT_URL = "http://patchwork/xmlrpc/" > CONFIG_FILE = os.path.expanduser('~/.pwclientrc') > > -class Filter: > +class Filter(object): > """Filter for selecting patches.""" > def __init__(self): > # These fields refer to specific objects, so they are special > @@ -135,7 +151,7 @@ def person_ids_by_name(rpc, name): > if len(name) == 0: > return [] > people = rpc.person_list(name, 0) > - return map(lambda x: x['id'], people) > + return [x['id'] for x in people] > > def list_patches(patches, format_str=None): > """Dump a list of patches to stdout.""" > @@ -356,7 +372,7 @@ class _RecursiveHelpAction(argparse._HelpAction): > parser.exit() > > def main(): > - hash_parser = argparse.ArgumentParser(add_help=False, version=False) > + hash_parser = argparse.ArgumentParser(add_help=False) > hash_parser.add_argument( > '-h', metavar='HASH', dest='hash', action='store', > help='''Lookup by patch hash''' > @@ -370,7 +386,7 @@ def main(): > help='''Lookup patch in project''' > ) > > - filter_parser = argparse.ArgumentParser(add_help=False, version=False) > + filter_parser = argparse.ArgumentParser(add_help=False) > filter_parser.add_argument( > '-s', metavar='STATE', > help='''Filter by patch state (e.g., 'New', 'Accepted', etc.)''' > @@ -409,7 +425,7 @@ def main(): > 'patch_name', metavar='STR', nargs='?', > help='substring to search for patches by name', > ) > - help_parser = argparse.ArgumentParser(add_help=False, version=False) > + help_parser = argparse.ArgumentParser(add_help=False) > help_parser.add_argument( > '--help', action='help', help=argparse.SUPPRESS, > #help='''show this help message and exit''' > @@ -418,7 +434,6 @@ def main(): > action_parser = argparse.ArgumentParser( > prog='pwclient', > add_help=False, > - version=False, > formatter_class=argparse.RawDescriptionHelpFormatter, > epilog='''(apply | get | info | view | update) (-h HASH | ID [ID > ...])''', > ) > -- > 2.5.2 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
> > From: Mike Frysinger <vapier@chromium.org> > > > > This fixes a few random issues to make the script work at least somewhat > > under python 3: > > - set the default encoding to utf-8 > > - handle xmlrpclib/xmlrpc.client module renames > > - handle ConfigParser/configparser module renames > > - add a unicode() stub for python 3 > > - fix old style class definition w/Filter > > - use list comprehension instead of map() > > - drop the unused version= keyword w/argparse > > > > The code still runs under python 2 the same as before, and now works for > > the most part under python 3 -- the handling of encoded content still > needs > > some work, but that'll require more surgery, and is best left to another > > commit after this. > > You're right - it needs a little more work but it's most of the way there. > Do you plan to do this? I was looking at making use of Django's provided > 'six' library to solve this issue for the server - we could use it here > also. Regardless, for this patch > > Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> > > > Signed-off-by: Mike Frysinger <vapier@chromium.org> Merged.
On Fri, Oct 16, 2015 at 05:15:31PM -0400, Mike Frysinger wrote: > From: Mike Frysinger <vapier@chromium.org> > > This fixes a few random issues to make the script work at least somewhat > under python 3: > - set the default encoding to utf-8 > - handle xmlrpclib/xmlrpc.client module renames > - handle ConfigParser/configparser module renames > - add a unicode() stub for python 3 > - fix old style class definition w/Filter > - use list comprehension instead of map() > - drop the unused version= keyword w/argparse > > The code still runs under python 2 the same as before, and now works for > the most part under python 3 -- the handling of encoded content still needs > some work, but that'll require more surgery, and is best left to another > commit after this. > > Signed-off-by: Mike Frysinger <vapier@chromium.org> Looks good to me, though I'm still a little green on Python 2/3 compatibility. I've been runnning this (plus other patches) since you first sent it out. Acked-by: Brian Norris <computersforpeace@gmail.com> There's still a little more low-hanging fruit for Python 3 compatbility (I have a few mashed up patches for that still), but this is a good start. (... and I see it was merged already. Cool.) Brian > --- > patchwork/bin/pwclient | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient > index d096f83..d2b95e5 100755 > --- a/patchwork/bin/pwclient > +++ b/patchwork/bin/pwclient > @@ -1,4 +1,5 @@ > #!/usr/bin/env python > +# -*- coding: utf-8 -*- > # > # Patchwork command line client > # Copyright (C) 2008 Nate Case <ncase@xes-inc.com> > @@ -23,16 +24,31 @@ from __future__ import print_function > > import os > import sys > -import xmlrpclib > +try: > + import xmlrpclib > +except ImportError: > + # Python 3 has merged/renamed things. > + import xmlrpc.client as xmlrpclib > import argparse > import string > import tempfile > import subprocess > import base64 > -import ConfigParser > +try: > + import ConfigParser > +except ImportError: > + # Python 3 has renamed things. > + import configparser as ConfigParser > import shutil > import re > > +# Add a shim for Python 2's unicode() helper. > +try: > + unicode > +except NameError: > + # Python 3 does everything by unicode now. > + unicode = str > + > # Default Patchwork remote XML-RPC server URL > # This script will check the PW_XMLRPC_URL environment variable > # for the URL to access. If that is unspecified, it will fallback to > @@ -40,7 +56,7 @@ import re > DEFAULT_URL = "http://patchwork/xmlrpc/" > CONFIG_FILE = os.path.expanduser('~/.pwclientrc') > > -class Filter: > +class Filter(object): > """Filter for selecting patches.""" > def __init__(self): > # These fields refer to specific objects, so they are special > @@ -135,7 +151,7 @@ def person_ids_by_name(rpc, name): > if len(name) == 0: > return [] > people = rpc.person_list(name, 0) > - return map(lambda x: x['id'], people) > + return [x['id'] for x in people] > > def list_patches(patches, format_str=None): > """Dump a list of patches to stdout.""" > @@ -356,7 +372,7 @@ class _RecursiveHelpAction(argparse._HelpAction): > parser.exit() > > def main(): > - hash_parser = argparse.ArgumentParser(add_help=False, version=False) > + hash_parser = argparse.ArgumentParser(add_help=False) > hash_parser.add_argument( > '-h', metavar='HASH', dest='hash', action='store', > help='''Lookup by patch hash''' > @@ -370,7 +386,7 @@ def main(): > help='''Lookup patch in project''' > ) > > - filter_parser = argparse.ArgumentParser(add_help=False, version=False) > + filter_parser = argparse.ArgumentParser(add_help=False) > filter_parser.add_argument( > '-s', metavar='STATE', > help='''Filter by patch state (e.g., 'New', 'Accepted', etc.)''' > @@ -409,7 +425,7 @@ def main(): > 'patch_name', metavar='STR', nargs='?', > help='substring to search for patches by name', > ) > - help_parser = argparse.ArgumentParser(add_help=False, version=False) > + help_parser = argparse.ArgumentParser(add_help=False) > help_parser.add_argument( > '--help', action='help', help=argparse.SUPPRESS, > #help='''show this help message and exit''' > @@ -418,7 +434,6 @@ def main(): > action_parser = argparse.ArgumentParser( > prog='pwclient', > add_help=False, > - version=False, > formatter_class=argparse.RawDescriptionHelpFormatter, > epilog='''(apply | get | info | view | update) (-h HASH | ID [ID ...])''', > ) > -- > 2.5.2 >
> (... and I see it was merged already. Cool.) I had reviewed (and have also been using) the patches 1[1] and 3[2] in the series since the first time they were sent. Had the same trivial comment about the second one but I let it slip - it's a good fix and I'm eager to get this kind of stuff in and backlog cleared out. > Looks good to me, though I'm still a little green on Python 2/3 > compatibility. I've been runnning this (plus other patches) since you What are your thoughts on the use of 'six' for this stuff? Would it help you here? Stephen [1] https://patchwork.ozlabs.org/patch/468579/ [2] https://patchwork.ozlabs.org/patch/468568/
On Fri, Oct 16, 2015 at 10:49:04PM +0000, Finucane, Stephen wrote: > > (... and I see it was merged already. Cool.) > > I had reviewed (and have also been using) the patches 1[1] and 3[2] in > the series since the first time they were sent. Had the same trivial > comment about the second one but I let it slip - it's a good fix and > I'm eager to get this kind of stuff in and backlog cleared out. Glad you're clearing the backlog! > > Looks good to me, though I'm still a little green on Python 2/3 > > compatibility. I've been runnning this (plus other patches) since you > > What are your thoughts on the use of 'six' for this stuff? Would it help you here? I'm not very familiar, but before reading this email I was trying to figure out the "best" interoperable way to replace dict.iteritems(), and I see we could use six.iteritems(). So my initial review says "sure"! Except then, we rely on PyPy and nonstandard libraries which doesn't seem too good to me for such a small utility :( Brian
On 16 Oct 2015 16:01, Brian Norris wrote: > On Fri, Oct 16, 2015 at 10:49:04PM +0000, Finucane, Stephen wrote: > > > Looks good to me, though I'm still a little green on Python 2/3 > > > compatibility. I've been runnning this (plus other patches) since you > > > > What are your thoughts on the use of 'six' for this stuff? Would it help you here? > > I'm not very familiar, but before reading this email I was trying to > figure out the "best" interoperable way to replace dict.iteritems(), and > I see we could use six.iteritems(). So my initial review says "sure"! dict.items() should work fine in both. the question becomes whether the dict is so large that coercing it to a list first adds significant overhead. i've found that generally it doesn't. > Except then, we rely on PyPy and nonstandard libraries which doesn't > seem too good to me for such a small utility :( six is a pretty common module. i wouldn't worry about requiring it. -mike
On 16 Oct 2015 22:13, Finucane, Stephen wrote: > > From: Mike Frysinger <vapier@chromium.org> > > > > This fixes a few random issues to make the script work at least somewhat > > under python 3: > > - set the default encoding to utf-8 > > - handle xmlrpclib/xmlrpc.client module renames > > - handle ConfigParser/configparser module renames > > - add a unicode() stub for python 3 > > - fix old style class definition w/Filter > > - use list comprehension instead of map() > > - drop the unused version= keyword w/argparse > > > > The code still runs under python 2 the same as before, and now works for > > the most part under python 3 -- the handling of encoded content still needs > > some work, but that'll require more surgery, and is best left to another > > commit after this. > > You're right - it needs a little more work but it's most of the way there. Do you plan to do this? I was looking at making use of Django's provided 'six' library to solve this issue for the server - we could use it here also. Regardless, for this patch django ? you mean this one right: https://pypi.python.org/pypi/six i don't think that's related to django .. i didn't have any plans to hack on pwclient at this point. i don't use it that frequently ;). -mike
> On 16 Oct 2015 16:01, Brian Norris wrote: > > On Fri, Oct 16, 2015 at 10:49:04PM +0000, Finucane, Stephen wrote: > > > > Looks good to me, though I'm still a little green on Python 2/3 > > > > compatibility. I've been runnning this (plus other patches) since you > > > > > > What are your thoughts on the use of 'six' for this stuff? Would it > help you here? > > > > I'm not very familiar, but before reading this email I was trying to > > figure out the "best" interoperable way to replace dict.iteritems(), and > > I see we could use six.iteritems(). So my initial review says "sure"! > > dict.items() should work fine in both. the question becomes whether the > dict > is so large that coercing it to a list first adds significant overhead. > i've > found that generally it doesn't. `list(dict.items())` to keep it compatible with Python 3, right? It's safe to say that exceptionally few developers are going to have machines with <512MB RAM today: we're good here. > > Except then, we rely on PyPy and nonstandard libraries which doesn't > > seem too good to me for such a small utility :( > > six is a pretty common module. i wouldn't worry about requiring it. > -mike Nah, I think he's right here. At least for the server it's packaged with Django but for users downloading pwclient from the server all they get is a single executable file. It's another barrier to entry if they need to install six before they can use it. I'm going to investigate (emphasis on investigate haha) the possibility of moving pwclient out of tree so it might be an option then but otherwise I guess we should stick to the hard 'if VERSION' checks, ugly though they may be. Stephen
diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient index d096f83..d2b95e5 100755 --- a/patchwork/bin/pwclient +++ b/patchwork/bin/pwclient @@ -1,4 +1,5 @@ #!/usr/bin/env python +# -*- coding: utf-8 -*- # # Patchwork command line client # Copyright (C) 2008 Nate Case <ncase@xes-inc.com> @@ -23,16 +24,31 @@ from __future__ import print_function import os import sys -import xmlrpclib +try: + import xmlrpclib +except ImportError: + # Python 3 has merged/renamed things. + import xmlrpc.client as xmlrpclib import argparse import string import tempfile import subprocess import base64 -import ConfigParser +try: + import ConfigParser +except ImportError: + # Python 3 has renamed things. + import configparser as ConfigParser import shutil import re +# Add a shim for Python 2's unicode() helper. +try: + unicode +except NameError: + # Python 3 does everything by unicode now. + unicode = str + # Default Patchwork remote XML-RPC server URL # This script will check the PW_XMLRPC_URL environment variable # for the URL to access. If that is unspecified, it will fallback to @@ -40,7 +56,7 @@ import re DEFAULT_URL = "http://patchwork/xmlrpc/" CONFIG_FILE = os.path.expanduser('~/.pwclientrc') -class Filter: +class Filter(object): """Filter for selecting patches.""" def __init__(self): # These fields refer to specific objects, so they are special @@ -135,7 +151,7 @@ def person_ids_by_name(rpc, name): if len(name) == 0: return [] people = rpc.person_list(name, 0) - return map(lambda x: x['id'], people) + return [x['id'] for x in people] def list_patches(patches, format_str=None): """Dump a list of patches to stdout.""" @@ -356,7 +372,7 @@ class _RecursiveHelpAction(argparse._HelpAction): parser.exit() def main(): - hash_parser = argparse.ArgumentParser(add_help=False, version=False) + hash_parser = argparse.ArgumentParser(add_help=False) hash_parser.add_argument( '-h', metavar='HASH', dest='hash', action='store', help='''Lookup by patch hash''' @@ -370,7 +386,7 @@ def main(): help='''Lookup patch in project''' ) - filter_parser = argparse.ArgumentParser(add_help=False, version=False) + filter_parser = argparse.ArgumentParser(add_help=False) filter_parser.add_argument( '-s', metavar='STATE', help='''Filter by patch state (e.g., 'New', 'Accepted', etc.)''' @@ -409,7 +425,7 @@ def main(): 'patch_name', metavar='STR', nargs='?', help='substring to search for patches by name', ) - help_parser = argparse.ArgumentParser(add_help=False, version=False) + help_parser = argparse.ArgumentParser(add_help=False) help_parser.add_argument( '--help', action='help', help=argparse.SUPPRESS, #help='''show this help message and exit''' @@ -418,7 +434,6 @@ def main(): action_parser = argparse.ArgumentParser( prog='pwclient', add_help=False, - version=False, formatter_class=argparse.RawDescriptionHelpFormatter, epilog='''(apply | get | info | view | update) (-h HASH | ID [ID ...])''', )