Proposal to add util.parseargs

[athomas]

It seems that almost every macro has to reimplement its own argument parser. This doesn't seem ideal.

I've implemented a generic argument parser for the tags plugin, available here. It parses arguments like this: kw=(a, b, c), foo, bar, and returns a tuple of (positional[], keywords{}).

If you think this is a good idea, I can clean it up for submission.

[cboos]

+1 on this.

But access to the unprocessed "arguments" (i.e. content) should be preserved, in case the arguments are not really arguments... e.g.

[[TODO(Do it, or do it not)]]

I wonder if we couldn't distinguish this kind of macro (as well as stay compatible with older macros) simply by looking at the arity of the render_macro method:

class IWikiMacroProvider(Interface):
    ...
    def render_macro(req, name, content):
        """Return the HTML output of the macro."""

    def render_macro(req, name, *args, **kwd):
        """Same as above, but use prepared arguments"""

(first form has an arity of 3, second of 2)

Alternatively, we could keep the render_macro(req, name, content) form and make use of an utility function, e.g.

    def render_macro(req, name, content):
        args, kwd = parse_args(content)

Last comment on this: this ideally should also take care of quote/double quotes, like in:

[[Image(coolpic.PNG, title="Ain't that nice, dude?")]]

I'm setting the prio/milestone for this, as it would really help me to rework a bit the Image macro...

[athomas]

I think having a utility function is preferable, so old-style macros can use it. The linked parseargs module does indeed handle quoted strings, though naively at the moment. eg.

3> from tractags.parseargs import parseargs
4> parseargs('''a="alec thomas", b=10''')
<4 ([], {'a': 'alec thomas', 'b': '10'})
5> parseargs('''a="ain't it nice", b=10''')
<5 ([], {'a': "ain't it nice", 'b': '10'})

By naively I mean it doesn't handle escaped (\) characters. I can update it to do so.

[athomas]

Another function that I thought would be useful in a number of places is the getbool() method from config. In fact, I have factored this out in workflow as trac.util.deserialize_bool().

[cboos]

Yes, but why not simpler names :to_bool and to_list?

[athomas]

Sure, I'm not fussed :)

I only chose deserialize primarily because there was no chance of ambiguity.

[athomas]

I renamed them in the latest workflow commit. It is much more convenient, can't deny it.

[athomas]

How about this implementation. It handles normal Python basic types and has the same calling convention and return type as the above.

def parse_args(args):
    import compiler

    def get_value(node):
        if isinstance(node, compiler.ast.Name):
            return node.name
        elif isinstance(node, compiler.ast.Const):
            return node.value
        elif isinstance(node, compiler.ast.Tuple) or \
             isinstance(node, compiler.ast.List):
            return tuple([get_value(el) for el in node.nodes])
        elif isinstance(node, compiler.ast.Dict):
            return dict([(get_value(k), get_value(v)) for k, v in node.items])
        raise ValueError('invalid arguments near %s' % str(node))

    largs = []
    kwargs = {}
    expr = compiler.parse('mock(%s)' % args, 'eval')
    for node in expr.getChildNodes()[0].args:
        if isinstance(node, compiler.ast.Keyword):
            kwargs[node.name] = get_value(node.expr)
        else:
            largs.append(get_value(node))
    return largs, kwargs

[cboos]

... and handles escaped characters and looks much cleaner & shorter than the first proposal. I like it.

But should we really put that in util? What other use than for macros do you foresee?

I think we should rather put it in trac/wiki/api.py, so that it can be imported together with IWikiMacroProvider (and maybe WikiMacroBase could be moved there too?)

[athomas]

Yes, agreed. Chris and I had come to the same conclusion in #trac.

[cboos]

So what about committing the above?

[athomas]

Sounds good to me, but I don't have commit access to trunk :)

[cboos]

You really should :)

In the meantime, I'll take care of this one for you.

[cboos]

Well, I did some additional testing, and it appears that this needs some more work before it can really be used on a bigger scale:

macros.py

@@ -54 +54 @@
         raise NotImplementedError
 
 
+def parse_args(args):
+    """Utility for parsing the `content` parameter in `render_macro`.
+
+    This follows the usual Python convention for parsing function
+    arguments, including named arguments:
+
+    >>> parse_args('') 
+    ([], {})
+    >>> parse_args('1, text') 
+    ([1, 'text'], {})
+    >>> parse_args('option=yes')
+    ([], {'option': 'yes'})
+    >>> parse_args('1, text, option=yes')
+    ([1, 'text'], {'option': 'yes'})
+
+    List and tuple arguments are supported too:
+
+    >>> parse_args('[1,2,3], list=(a,b)')
+    ([(1, 2, 3)], {'list': ('a', 'b')})
+
+    It's also very useful for handling quotes the expected way:
+
+    >>> parse_args('"foo, bar", color="rgb(ff,00,00)", separator=","')
+    (['foo, bar'], {'color': 'rgb(ff,00,00)', 'separator': ','})
+
+    '''However'''
+
+    A big limitation of this approach is that any unquoted text argument
+    has to be a proper Python identifier.
+    i.e. it can't contain space, start with a digit, etc.
+
+    A lot of expected usage of macros wouldn't work.
+
+    >>> parse_args('Hello world!')
+    Traceback (most recent call last):
+         ...
+    SyntaxError: invalid syntax
+
+    >>> parse_args('Prefix%%')             # NB: second % for doctest's sake
+    Traceback (most recent call last):
+         ...
+    SyntaxError: invalid syntax
+
+    >>> parse_args('TracDev/')
+    Traceback (most recent call last):
+         ...
+    SyntaxError: unexpected EOF while parsing
+
+    >>> parse_args('OtherPage:foo.bmp')
+    Traceback (most recent call last):
+         ...
+    SyntaxError: invalid syntax
+
+    >>> parse_args('photo.jpg, align=right')
+    Traceback (most recent call last):
+         ...
+    ValueError: invalid arguments near Getattr(Name('photo'), 'jpg')
+    """
+    import compiler
+        
+    def get_value(node):
+        if isinstance(node, compiler.ast.Name):
+            return node.name
+        elif isinstance(node, compiler.ast.Const):
+            return node.value
+        elif isinstance(node, compiler.ast.Tuple) or \
+             isinstance(node, compiler.ast.List):
+            return tuple([get_value(el) for el in node.nodes])
+        elif isinstance(node, compiler.ast.Dict):
+            return dict([(get_value(k), get_value(v)) for k, v in node.items])
+        raise ValueError('invalid arguments near %s' % str(node))
+
+    largs = []
+    kwargs = {}
+    expr = compiler.parse('mock(%s)' % args, 'eval')
+    for node in expr.getChildNodes()[0].args:
+        if isinstance(node, compiler.ast.Keyword):
+            kwargs[node.name] = get_value(node.expr)
+        else:
+            largs.append(get_value(node))
+    return largs, kwargs
+
+
 class TitleIndexMacro(WikiMacroBase):
     """Inserts an alphabetic list of all wiki pages into the output.

[athomas]

Yes of course. The rewritten version was not meant as a general backwards compatible solution, but for future macros that require Python-esque arguments.

The issues you ran into are exactly why the parseargs from the tags plugin is so much more complex: it handles the parsing itself rather than relying on the Python parser, and consequently allows for more free-form arguments:

1> from tractags.parseargs import parseargs
2> parseargs('alec-thomas, thomas')
<2 (['alec-thomas', 'thomas'], {})
3> parseargs('alec!!!, thomas')
<3 (['alec!!!', 'thomas'], {})
4> parseargs('alec!!!=(1, 2, 3), thomas')
<4 (['thomas'], {'alec!!!': ['1', '2', '3']})

It doesn't support spaces in arguments, but could be modified to do so.

[cboos]

Ok to postpone this to 0.11?

[athomas]

Yeah I agree.

[cboos]

What about this simpler parse_args() ? The rules would be:

• every "," character inside a macro call is an argument separator, unless escaped by a "\"
• every identifier directly followed by an "=" corresponds to an assignement to a named argument

The point being that Wiki authors won't necessarily want to follow Python syntactic rules when writing macros, and that the simple rules above are backward compatible, unambiguous, and simple to implement.

def parse_args(args):
    largs, kwargs = [], {}
    if args:
        for arg in re.split(r'(?<!\\),', args):
            m = re.match(r'\s*[a-zA-Z_]\w=', arg)
            if m:
                kwargs[arg[:m.end()-1]] = arg[m.end():]
            else:
                largs.append(arg)
    return largs, kwargs

Typical use, e.g. for the last arguments of the TicketQuery macro (the first one being always the query itself):

>>> parse_args('format=compact, order=priority, title=Priority List')
([], {' title': 'Priority List', ' order': 'priority', 'format': 'compact'})

[cboos]

Hm, well, the simple parse_args from above slipped in with r3694. I think that's enough to cover most needs.

[martin@…]

I like to suggest the following enhancement to parse_args, which allows non-escaped commas inside double-quotes, e.g.:

[[Macro(address="My street, city, county",telephon=12345)]]

This can be done by changing the split regex (see patch below). I found a suitable one on this website.

I tested it a little in the console with different non-trivial sets, but further testing should be done.

There is also the possibility to introduce a new parameter to parse_args which controls if the old or the new regex is used to ensure 100% backward compatibility for unforeseen cases where the new regex might fail, e.g.:

 largs, kwargs = parse_args(content, quotes=True)

api.

@@ -139 +139 @@
     """    
     largs, kwargs = [], {}
     if args:
-        for arg in re.split(r'(?<!\\),', args):
+        for arg in re.split(r'(?<!\\),(?!(?:[^",]|[^"],[^"])+")/', args):
             arg = arg.replace(r'\,', ',')
             if strict:
                 m = re.match(r'\s*[a-zA-Z_]\w+=', arg)

[cboos]

Will have a look at it, but not for 0.11.2.

[martin@…]

Replying to martin@…:

+ for arg in re.split(r'(?<!\\),(?!(?:[^",]|["],[^{"])+")/', args):}

I just figured out that this regex actually doesn't work with python in the current form. It does work with Ruby, PHP and Perl, however. The '[",]' seems to be the problem. Sorry, but I used Perl in my earlier test, because it's easier for me and I thought the regex syntax is the same.

Anyway, I googled a little more and would like now to suggest the use of pythons csv module:

import csv
for arg in csv.reader([args],delimiter=',',escapechar='\\').next():

See http://www.python.org/doc/2.5.2/lib/module-csv.html for an description.

[martin@…]

I checked now the csv module more deeply and found out that while it supports the handling of comma-separated, possible quoted 'values' very well it doesn't work with partially quoted content, i.e. both key and value must be quoted together, not just the value part:

"key=value" instead of key="value", which isn't what we want. I rechecked the previous posted RegEx? and found out that it fails at the same issue.

[martin@…]

Some friends of mine and I now spend some time over this and we came to the solution that an own small parser, not a regex, is needed for the split operation to allow (non-escaped) commas in quoted values. Also the case were a back-slash should be the last character value of an argument (e.g. path=c:\windows\,other=pair) can be handled correctly by the parser, which isn't the case in the current implementation.

The below parser ignores commas in double quotes and lets the backslash escape any following character (e.g. path=c:\\windows\\,other=pair would result in { 'path': r'c:\windows\', 'other':'pair }).

def parse_args (args,strict=True):
    """ parses a comma separated string were the values can include multiple
    quotes """
    esc   = 0   # last char was escape char
    quote = 0   # inside quote
    start = 0   # start of current field
    pos   = 0   # current position
    largs  = []
    kwargs = {}

    def checkkey (arg):
        import re
        arg = arg.replace(r'\,', ',')
        if strict:
            m = re.match(r'\s*[a-zA-Z_]\w+=', arg)
        else:
            m = re.match(r'\s*[^=]+=', arg)
        if m:
            kw = arg[:m.end()-1].strip()
            if strict:
                kw = unicode(kw).encode('utf-8')
            kwargs[kw] = arg[m.end():]
        else:
            largs.append(arg)

    if args:
        # Small parser to split on commas
        #  * Ignores commas in double quotes
        #  * Backslash escapes any following character
        for char in args:
            if esc:
                esc = 0
            elif char == '"':
                quote = not quote
            elif char == '\\':
                esc = 1
            elif char == ',' and not quote:
                checkkey( args[start:pos] )
                start = pos + 1
            pos = pos + 1
        checkkey( args[start:] )
    return largs, kwargs

In addition the quote, escape and delimiter characters could be made optional arguments to further generalize the parser. Some special macro might need to re-parse single values with other delimiters which might also include quotes (for example my th:GoogleMapMacro macro for the marker argument).