Unify wiki processors and macros

[Remy Blank]

Currently, wiki macros can be called in two different ways:

• Macro syntax: [[Macro(params)]]
• Wiki processor syntax: {{{#!Macro}}}

Both ways also allow calling MIME renderers and special-purpose processors, defined in WikiProcessor.

Wiki processors and macros are actually almost the same thing, except for the following differences:

• Wiki processors can take additional arguments, defined on the #! line. When calling a macro using the processor syntax, these arguments are lost.

• There is no extension point for defining wiki processors, they have to be implemented in WikiProcessor.

Instead of adding a new extension point to define wiki processors, I'd like to add an additional argument to IWikiMacroProvider.expand_macro() to pass the arguments of the wiki processor syntax. The interface would become:

def expand_macro(formatter, name, content, args=None):
    pass

When called with the macro syntax, args would be None. Otherwise, it would pass the .args attribute of WikiProcessor, which is a dictionary of arguments.

The existing processors defined in WikiProcessor could then be moved out into their own macro component, but this is optional.

To ensure backwards compatibility with existing macros and call sites, the following measures will be taken:

• The existing expand_macro call sites will check if the method expects the additional argument or not, and only pass it in the former case.

• The new args argument should have a default value of None, to ensure that call sites not in Trac core that still expect the old interface continue to work.

I should have a patch ready in a few days. Comments welcome.

[Remy Blank]

Pass wiki processor arguments to macros.

[Remy Blank]

The patch above adds the new optional argument args to IWikiMacroProvider.expand_macro(), though it uses a default of {}, as WikiProcessor is also used for expanding [[Macro()]] syntax. So there's currently no way of differentiating between both forms of calls, which IMO is actually a good thing, as a macro should be written for either of the syntaxes, but normally not both.

The whole change is completely backwards compatible, and will allow implementing the commitlog macro for #1507.

The patch also fixes a minor off-by-two error in Formatter, where the #! at the start of a processor call was not taken into account (it doesn't make a difference, though, as the first item in the result list is dropped anyway).

[Christian Boos]

Patch reviewed, ok to apply.

Don't forget to update TracDev/ApiChanges/0.12.

[Remy Blank]

Thanks, applied in [8372].

[Remy Blank]

And documentation updated in TracDev/ApiChanges/0.12@2.

[Christian Boos]

Concerning the additional argument 'class': 'wikipage', I think I should fix that, by adding class='wikipage' in the div processor only:

trac/wiki/formatter.py

@@ -157 +157 @@
         return elt
 
     def _div_processor(self, text):
+        if 'class' not in self.args:
+            self.args['class'] = 'wikipage'
         return self._elt_processor('div', format_to_html, text, self.args)
 
     def _span_processor(self, text):
@@ -802 +804 @@
                 values = [(v and v[0] in '"\'' and [v[1:-1]] or [v])[0]
                           for v in args[1::2]]
                 args = dict(zip(keys, values))
-                if 'class' not in args:
-                    args['class'] = 'wikipage'
                 self.code_processor = WikiProcessor(self, name, args)
             else:
                 self.code_buf.append(line)

What do you think?

[Remy Blank]

I was indeed wondering if that argument was used elsewhere, but haven't taken the time to look. It does look like the div processor is the only user, so yes, please go ahead. I'll remove the comment from the API change documentation.

[Christian Boos]

Replying to rblank: Ok, applied in [8374]

[Christian Boos]

Replying to rblank:

The patch above adds the new optional argument args to IWikiMacroProvider.expand_macro(), though it uses a default of {},

Putting {} (or []) as the default value for a parameter is almost always a bug. I could have fixed it by assigning {} to self.args later, so that the code retains the following property:

there's currently no way of differentiating between both forms of calls, which IMO is actually a good thing, as a macro should be written for either of the syntaxes, but normally not both.

But I think that being able to differentiate between those could be useful, as switching between both styles of call makes sense in some cases. A typical example would be the span processor, which should also become callable using a #!span block.

Hence wikiprocessors-or-macros-r9193.patch​ and span-blocks-r9193.patch​.

[Christian Boos]

WikiProcessors: fix a potential problem with processor arguments.

[Christian Boos]

WikiProcessors: span can be called as a wiki processor.

[Remy Blank]

Replying to cboos:

Putting {} (or []) as the default value for a parameter is almost always a bug.

That's a bit an exaggeration IMO, it's fine as long as you don't mutate the argument. But ok.

But I think that being able to differentiate between those could be useful, as switching between both styles of call makes sense in some cases. A typical example would be the span processor, which should also become callable using a #!span block.

Wasn't that already the case? IIRC, you can call the macro with both [[Span()]] and #!Span. Ah, except that you couldn't use the processor arguments for attributes, but you had to place them in the content (as was the case before). Ok, that's pretty useful (and an oversight on my part). Patches look good, please apply. Maybe also change the interface definition to show a default argument of None instead of {}?

[Christian Boos]

Replying to rblank:

Replying to cboos:

Putting {} (or []) as the default value for a parameter is almost always a bug.

That's a bit an exaggeration IMO, it's fine as long as you don't mutate the argument.

… which can be done in a different place that in the method in question, as was the case there, so this can make things a bit more difficult to maintain. I think we should better avoid it, unless we really, really, REALLY need it ​:-).

Patches look good, please apply. Maybe also change the interface definition to show a default argument of None instead of {}?

Done in r9199 (with docstrings updated) and r9200.

[Remy Blank]

Replying to cboos:

… which can be done in a different place that in the method in question, as was the case there, so this can make things a bit more difficult to maintain.

Sure. What I meant is that it is not a bug per se, rather a "bug waiting to happen" ;-)