Patchwork [v3] command options: abort on unicode defaults

login
register
mail settings
Submitter Christophe de Vienne
Date Sept. 1, 2017, 7:44 a.m.
Message ID <06bc5adebc48cfb657cd.1504251864@needle>
Download mbox | patch
Permalink /patch/23588/
State Changes Requested
Headers show

Comments

Christophe de Vienne - Sept. 1, 2017, 7:44 a.m.
# HG changeset patch
# User Christophe de Vienne <christophe@cdevienne.info>
# Date 1504023891 -7200
#      Tue Aug 29 18:24:51 2017 +0200
# Node ID 06bc5adebc48cfb657cdefda41704a96f686fae7
# Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
command options: abort on unicode defaults

If the default value of an option is a unicode string (something
than happen easily when using a 'from __future__ import unicode_literals'),
any value passed on the command line will be ignored because the fancyopts
module only checks for byte strings and not unicode strings.

Changing fancyopts behavior is easy but would make assumptions on how
the python3 port should be done, which is outside the scope of this patch.

The chosen approach is to abort when a unicode default value is detected,
with a hint for the developer.
Yuya Nishihara - Sept. 1, 2017, 12:50 p.m.
On Fri, 01 Sep 2017 09:44:24 +0200, Christophe de Vienne wrote:
> # HG changeset patch
> # User Christophe de Vienne <christophe@cdevienne.info>
> # Date 1504023891 -7200
> #      Tue Aug 29 18:24:51 2017 +0200
> # Node ID 06bc5adebc48cfb657cdefda41704a96f686fae7
> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> command options: abort on unicode defaults
> 
> If the default value of an option is a unicode string (something
> than happen easily when using a 'from __future__ import unicode_literals'),
> any value passed on the command line will be ignored because the fancyopts
> module only checks for byte strings and not unicode strings.
> 
> Changing fancyopts behavior is easy but would make assumptions on how
> the python3 port should be done, which is outside the scope of this patch.
> 
> The chosen approach is to abort when a unicode default value is detected,
> with a hint for the developer.
> 
> diff -r b2eb0aa445cb -r 06bc5adebc48 mercurial/dispatch.py
> --- a/mercurial/dispatch.py	Tue Aug 22 21:21:43 2017 -0400
> +++ b/mercurial/dispatch.py	Tue Aug 29 18:24:51 2017 +0200
> @@ -558,6 +558,14 @@
>              args = pycompat.maplist(
>                  util.expandpath, pycompat.shlexsplit(defaults)) + args
>          c = list(entry[1])
> +        for option in entry[1]:
> +            default = option[2]

Maybe this can be checked at fancyopts where the options table is processed.
Alternatively, it could be added to extensions._validatecmdtable().

> +            if not pycompat.ispy3 and isinstance(default, type(u'')):

This can be enabled on Python 3. unicode strings should be banned.

> +                raise error.Abort(

error.ProgrammingError?

> +                    "A unicode default value (%s) was passed to the "
> +                    "%s.%s option" % (repr(default), cmd, option[1]),
> +                    "You should change the %s.%s default value to a "
> +                    "non-unicode string" % (cmd, option[1]))

Patch

diff -r b2eb0aa445cb -r 06bc5adebc48 mercurial/dispatch.py
--- a/mercurial/dispatch.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/dispatch.py	Tue Aug 29 18:24:51 2017 +0200
@@ -558,6 +558,14 @@ 
             args = pycompat.maplist(
                 util.expandpath, pycompat.shlexsplit(defaults)) + args
         c = list(entry[1])
+        for option in entry[1]:
+            default = option[2]
+            if not pycompat.ispy3 and isinstance(default, type(u'')):
+                raise error.Abort(
+                    "A unicode default value (%s) was passed to the "
+                    "%s.%s option" % (repr(default), cmd, option[1]),
+                    "You should change the %s.%s default value to a "
+                    "non-unicode string" % (cmd, option[1]))
     else:
         cmd = None
         c = []
diff -r b2eb0aa445cb -r 06bc5adebc48 tests/test-dispatch.t
--- a/tests/test-dispatch.t	Tue Aug 22 21:21:43 2017 -0400
+++ b/tests/test-dispatch.t	Tue Aug 29 18:24:51 2017 +0200
@@ -60,3 +60,23 @@ 
   [255]
 
 #endif
+
+Test abort on command options with unicode default value
+
+  $ hg init $TESTTMP/opt-unicode-default
+
+  $ cat > $TESTTMP/test_unicode_default_value.py << EOF
+  > from mercurial import registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command('ext', [('', 'opt', u'value', u'help')], 'ext [OPTIONS]')
+  > def ext(*args, **opts):
+  >     print(opts['opt'])
+  > EOF
+  $ cat > $TESTTMP/opt-unicode-default/.hg/hgrc << EOF
+  > [extensions]
+  > test_unicode_default_value = $TESTTMP/test_unicode_default_value.py
+  > EOF
+  $ hg -R $TESTTMP/opt-unicode-default ext
+  abort: ("A unicode default value (u'value') was passed to the ext.opt option", 'You should change the ext.opt default value to a non-unicode string')
+  [255]