[PATCH] templater: show the style list when I try to use a wrong one

[Simon King]

On Thu, Apr 18, 2013 at 8:23 PM, Iulian Stana <julian.stana at gmail.com> wrote:
> # HG changeset patch
> # User Iulian Stana <julian.stana at gmail.com>
> # Date 1366312898 -10800
> #      Thu Apr 18 22:21:38 2013 +0300
> # Node ID 4d5818e25c6da500f3d2d67ffce47ea9662aa166
> # Parent  7d31f2e42a8afb54c8fae87e8e3e29a63578aea4
> templater: show the style list when I try to use a wrong one
>
> When someone try to use a wrong style, a list with sugestions will appear.
>
> In the test-log.t file it's a test that prove this thing.
>
> diff -r 7d31f2e42a8a -r 4d5818e25c6d mercurial/templater.py
> --- a/mercurial/templater.py    Mon Apr 15 18:57:04 2013 -0300
> +++ b/mercurial/templater.py    Thu Apr 18 22:21:38 2013 +0300
> @@ -392,6 +392,21 @@
>
>  engines = {'default': engine}
>
> +def stylelist():
> +    path = templatepath()[0]
> +    dirlist =  os.listdir(path)
> +    stylelist = ''
> +    notfirst = False
> +    for file in dirlist:
> +        split = file.split(".")
> +        if split[0] == "map-cmdline":
> +            if notfirst == True:
> +                stylelist = stylelist + ','
> +            else:
> +                notfirst = True
> +            stylelist = stylelist + ' ' + split[1]
> +    return stylelist
> +

There are a few python idioms that could help you here. One is the
"enumerate" function, which yields pairs of (index, item) tuples from
a sequence. You could use it like this:

for index, file in enumerate(dirlist):
    split = file.split(".")
    if split[0] == "map-cmdline":
        if index == 0:
            stylelist = stylelist + ','
    # etc.

Also, in Python, it's generally not considered good practice to build
strings by repeatedly adding lots of small strings. Instead it is
normally better to build a list, and then use the str.join method to
concatenate all the strings. This also has the benefit of not having
to treat the first item differently:

stylelist = []
for file in dirlist:
    split = file.split(".")
    if split[0] == "map-cmdline":
        stylelist.append(split[0])

return ", ".join(stylelist)

Finally, purely as a personal preference, I would have the stylelist()
function return a list rather than a string, and do the joining with
the comma in the calling function.

>  class templater(object):
>
>      def __init__(self, mapfile, filters={}, defaults={}, cache={},
> @@ -413,7 +428,7 @@
>          if not mapfile:
>              return
>          if not os.path.exists(mapfile):
> -            raise util.Abort(_('style not found: %s') % mapfile)
> +            raise util.Abort(_("style '%s' not found \n(available styles:%s)") % (mapfile,stylelist()))
>

I think what Kevin was trying to get you to do here was something like:

    raise util.Abort(_("style '%s' not found") % mapfile,
                     hint=_("available styles: %s") % stylelist())

Hope that helps,

Simon

>          conf = config.config()
>          conf.read(mapfile)
> diff -r 7d31f2e42a8a -r 4d5818e25c6d tests/test-log.t
> --- a/tests/test-log.t  Mon Apr 15 18:57:04 2013 -0300
> +++ b/tests/test-log.t  Thu Apr 18 22:21:38 2013 +0300
> @@ -84,6 +84,14 @@
>    abort: cannot follow file not in parent revision: "dir"
>    [255]
>
> +-f, a wrong style
> +
> +  $ hg log -f -l1 --style something
> +  abort: style 'something' not found
> +  (available styles: changelog, bisect, default, xml, compact)
> +  [255]
> +
> +
>  -f, but no args
>
>    $ hg log -f
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel