fixed template and parser function "=" semantic

[tacc-tacc]

@5j9 Sorry if I don't let you sleep, but I have an arsenal of PRs to cast one by one. Let's focus on the next problem: parser functions inherit template "=" semantic. My proposal is:

• Reorganize _parser_function.py. Why SubWikiTextArgs is on this file while it has to be used on both Template and ParserFunction class? Why not putting ParserFunction and Template at the same hierarchy, while both classes inherit SubWikiTextArgs from somewhere else? Then, moved SubWikiTextArgs to _argument.py. Now, both _template.oy and _parser_function.py require this parent class from the same file, which is a more natural organization.
• Redefined and restructured ParserFunction and Template classes. Now both will have a variable called _ignore_equals, which is a boolean that defines "=" behiavor. On parser functions "=" character is ignored.
• For ParserFunction, added methods set_arg, get_arg, del_arg and has_arg. These methods only work with numbers as argument names, since pf's can only have positional arguments (of course, you can consider #switch as an exception but omit that case for today :))
• For Template, reworked set_arg. What happens if you t.set_arg('2', 'a', positional=True) for t = {{t}} ? Well, I think the criteria was (and is, and I enforced):
  • If the name is None, then append a positional (unnamed) argument.
  • If the name is not None and is NEW then will be positional iff positional == True AND is integer and is right after last index of positional arguments (hence get_last_idx_positional_args).
  • If the name is not None and is ALREADY EXISTENT, then positional defines whether this argument has to be converted to positional (i.e., remove the name of the argument in the template). Converting from positional to keyword raises an exception.
• For ParserFunction, if set_arg is called with a not integer name, the function currently does nothing, but we can throw an exception if you prefer.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (0f7a416) to head (4b7aef3).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         2270      2296   +26     
=========================================
+ Hits          2270      2296   +26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tacc-tacc]

I can't see Codecov report, got error 400.

[tacc-tacc]

Ok I amended my commit but I still don't understand what's the problem with Codecov, it gets angry because of get_lists and _content_span despite I wrote tests for both lines :(

[tacc-tacc]

Ok I amended one more time, now I understand what was wrong. The _content_span property was already defined in SubWikitextWithArgs parent class, so the redefinition on children was redundant. Second, I forgot to test normal_name for parser functions. In pf's the spaces after the name and before : are not ignored, hence we just lstrip WS instead of doing a pure strip. The lower dashes are not converted into spaces, this is also different from templates I think.

[5j9]

Thanks! I like the overall direction here, especially moving SubWikiTextWithArgs into _argument.py and separating the parser-function argument handling from the template implementation.

One thing I'm not quite comfortable with is making _ignore_equals = True a property of ParserFunction as a whole. I agree that for most parser functions = should be treated as part of the argument value rather than as a name/value separator, but as you noted yourself, there are parser functions such as #switch and #tag where = does have a special meaning:

{{#switch: baz | foo = Foo | baz = Baz | Bar }}

{{#tag:ref|Citation...|name="multiple"}}

To make the implementation general enough to handle all cases, I'd suggest keeping Argument unchanged. Instead, add an ignore_equals parameter to ParserFunction.get_arg(), set_arg(), has_arg(), and del_arg(), defaulting to True (or even make it required). I'd also make it keyword-only.

For example, the API could look roughly like:

def get_arg(self, name, *, ignore_equals=True):
    ...

so that the caller can explicitly choose the semantics when needed:

pf = ParserFunction('{{#ifeq:a|b|c|d}}')
pf.set_arg('1', 'b', ignore_equals=True)

pf = ParserFunction('{{#switch:var|case1= 1 | case2 = 2 }}')
pf.set_arg('case2', 'b', ignore_equals=False)

Conceptually, ignore_equals=True would treat the whole argument as positional, while ignore_equals=False would interpret = as a name/value separator. This keeps the Argument class unchanged and lets the ParserFunction API handle the differences between parser functions without exposing the underlying Argument machinery.

---

Some other parser functions where = is treated as a separator in them:

• #babel: nocat=1
• #categorytree: mode=parents

[tacc-tacc]

@5j9 Would be viable to fetch a list of parser functions names where the "=" must not be ignored, and use this same list to define a default behavior of ParserFunction methods?

[5j9]

@5j9 Would be viable to fetch a list of parser functions names where the "=" must not be ignored, and use this same list to define a default behavior of ParserFunction methods?

Parser function names can be translated, so we'd have to keep track of all those translations across languages and over time. That would make the implementation and maintenance considerably more complicated, and I don't think it's worth it.
I'd rather have the caller specify the intended = semantics when needed than maintain a list of parser functions based on their names.

[5j9]

Also, #tag is a good example of why this can't be determined solely from the parser function name:

{{#tag:ref|Citation on = Magic words. |name = "multiple"}}

Here the second argument is always positional, even though its value contains =, while the third argument uses = as the name/value separator. So the = handling can depend on the position/type of the argument, not just the parser function name.

[tacc-tacc]

@5j9 I had to rework the Argument class since it assumes that "=" must not be ignored. So I unfolded the decorators into get and set methods, and merged the functions of Template and ParserFunction into SubWikiTextWithArgs. Confirm if this is what you were expecting for, please.

[tacc-tacc]

There is a problem with lists_shadow_ss, this is decorated and assumes "=" must not be ignored. However, if we change this to a getter, we would also have to do in parent class WikiText or get_lists will not work at all.

[5j9]

I don't think the existing name and value properties should be removed. Those are part of the existing Argument API, so removing them would be a breaking change, and unfortunately that's too much of a breaking change for this PR.

My original thought was that this could perhaps be implemented without changing Argument at all, by making the new ParserFunction methods use the existing internal machinery with the appropriate interpretation of =. I can see, though, that this may lead to duplicated or awkward/unabstracted code, which is probably why you ended up restructuring Argument.

If some addition to Argument is necessary, one possibility would be to add a content property (and corresponding setter if needed) whose semantics are always positional: i.e. it returns/sets the entire argument content without treating = as a name/value separator. Then the existing name and value properties can retain their current API and semantics, while parser functions can use content when an argument needs to be treated as positional even though it contains =.

I'm not sure yet whether content fits cleanly with the current implementation, so if you think it doesn't work for some reason, I'm happy to discuss other options, including adding specific methods. The important part for me is that the existing name/value API remains intact.

I'm not suggesting that name/value can never be changed. If there turns out to be a compelling reason that their API needs to be redesigned, we could deprecate them first and handle that as a separate API change. But I don't think that's necessary here, and I'd prefer to keep them unchanged for this PR.

Regarding lists_shadow_ss: I think there probably isn't a clean way around that particular issue with the current approach, because the argument itself has no way to know whether it is positional or keyword-style. I'd suggest leaving that part as it is for now. It is only used by get_lists(), and for parser functions get_lists() should probably always treat those arguments as non-keyword arguments anyway. That can be addressed in a separate patch.

More generally, I think it's fine if some tests or other parts of _wikitext.py need adjustment as a consequence of fixing the argument handling. That's expected with a change like this. I'd just prefer to keep this PR focused on the argument/parsing issue rather than making additional API or structural changes at the same time.

---

One other small point: moving SubWikiTextWithArgs into the arguments module seems like a good cleanup to me. I wonder if that part could be done as a separate PR, though. It would have made this diff smaller and easier to review, and I could probably have merged that cleanup independently before reviewing the parser-function changes.

That said, I don't want to discourage the refactoring — I think the move itself makes sense. I'm mainly thinking about keeping the current PR focused and making the substantive argument-handling changes easier to review.