fixed template and parser function "=" semantic - #150

Codecov Report

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

@@            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.

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

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 :(

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.

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

@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 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.

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.

@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.

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.

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 change to Argument is necessary, I'm wondering whether we actually need to change the existing name/value machinery at all. If the goal is simply to access or set the argument as uninterpreted positional content, that might be as simple as operating directly on arg.string — arg.string[1:] for the getter, and arg.string = arg.string[0] + value for the setter. I may be missing something in the current implementation, though, so if that doesn't work because of some bookkeeping or other constraint, I'm happy to discuss other options.

In other words, I think we should be able to preserve the existing name/value API while adding whatever minimal machinery is needed for the parser-function-specific interpretation of =.

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.

@5j9 I don't understand why not merge both Template and ParserFunction logic into parent SubWikiTextWithArgs. I think it is the best moment to do it; I doubt this class is ever used or accesed from the API. Regarding the Argument class, I understand that probably will be accessed because when you iterate a template is the object you have to interact with, and then it would break all the compatibility with the previous versions. So, I think a good compromise is what you said: leave Argument untouched as it was before and, inside SubWikiTextWithArgs logic, have an if clause that depending on ignore_equals value, accesses the Argument object from the decorators or from the .string attribute.

But both things come togheter, so if the problem is the "scope" of the PR, let there be cancelled so I open a "broader" PR.

Regarding get_lists_ss, it might be better to consider deprecating this method since it assumes that equals should not be ignored. Instead, it seems it is more proper to iterate through template arguments and call bare get_lists with either value or string attributes, depending on what you want.

Regarding merging the Template and ParserFunction logic into SubWikiTextWithArgs, I'm not sure what exactly you mean by “merge” here.

If you mean removing Template and ParserFunction as separate classes and having users interact with SubWikiTextWithArgs instead, then I don't think that's an option. That would be another breaking API change. I also don't see much benefit in doing that, since ParserFunction may eventually need parser-function-specific behavior of its own (for example, evaluation or other functionality), even if I don't have plans to implement that right now.

If you mean keeping Template and ParserFunction as public classes, but moving their common argument-related implementation into SubWikiTextWithArgs so that those classes become mostly thin wrappers, then that's something I can consider. I just want to make sure we're talking about the same thing before deciding on it.

Regarding the scope, I wouldn't worry too much about whether the PR is “too broad” in itself. My concern is mainly that a larger PR naturally takes longer to review, especially when it involves both a behavioral change and a substantial refactoring. So if you make it broader, that's fine; just keep in mind that it may take me longer to get through it.

I don't think there's a need to cancel the current PR just because of the scope. I'd rather first settle on what the desired API and structure should be, and then we can decide whether the current PR should be revised or made broader.

Regarding _lists_shadow_ss, I assume that's what you mean by get_lists_ss, since there is no public get_lists_ss method in the current main branch. _lists_shadow_ss is already private, so removing or changing it by itself isn't an API-breaking change; what matters is whether the resulting get_lists() behavior changes.

I think its current behavior is correct for Template, so I wouldn't change that just because parser functions need different semantics. One possibility would be for ParserFunction to override the relevant logic so that its arguments are always treated as ignoring =. If we do that, though, I'd want tests for the behavior, ideally including at least one real-world parser-function case where treating = that way is actually required.

So I think that part can be handled without changing the existing Template behavior, and I'd prefer to keep it focused on the argument-handling issue rather than deprecating or removing anything unnecessarily.

@5j9 What I mean is to add all the shared logic into parent class SubWikiTextWithArgs, a hidden class to the API/user side. And then, leave Template and ParserFunction as derived classes which will only override things that work different and are specific for each derived class, that I think it will be the initial : for the first parameter in case of pf's and the ignore_equals always False in Template. Like the current version of the PR. Then, revert Argument to the state it was before the PR and instead of calling Argument methods/getters/setters with the boolean flag, just add if statements inside SubWikiTextWithArgs that depending on ignore_equals accesses Argument from the decorators or from the string attribute.

Regarding _lists_shadow_ss, I think it is a misleading method and it might worth be considered to deprecate in the future. But that is a separate Issue/PR let's focus on the changes for this PR, as I said:

• Unify common logic of two childs into parent SubWikiTextWithArgs and leave both childs, where will be overrided only the things that are specifically different for each object.
• Add ParserFunction methods (trivial because 95% of the logic is the same that what was there on Template, so it will just be inherited from parent).
• Add ignore_equals non positional parameter which will be accepted only on ParserFunction methods. I think that should be a boolean that is passed to SubWikiTextWithArgs where will be the conditionals I mentioned, and will always be harcoded to False for Template
• Fix some problems for set_arg, related with ambiguities on code regarding 1st parameter (i.e. the same name of wiki parameter) value and positional behavior. Solve all the cases and ambiguities for all the possible combinations for "1st parameter" (name), positional and ignore_equals combinations when this method is called. (read the opening message for this PR)

OK?

Yes, that sounds mostly OK to me.

For SubWikiTextWithArgs, in general I prefer parser-function-specific logic to live in ParserFunction and template-specific logic to live in Template, rather than having the parent know about the differences between its children. But if keeping the common implementation in SubWikiTextWithArgs avoids substantial duplication, I think having a conditional there based on ignore_equals is a reasonable compromise.

I'm also fine with keeping _lists_shadow_ss as a separate issue/PR for now. ✔️

Regarding set_arg, this part is a little harder for me to follow from the description. I haven't reviewed the proposed logic yet, so I don't want to say that the particular behavior is OK before looking at it.

What I'd like to understand here is primarily what existing behavior is changing and why. The opening message describes what the new behavior should be, but it doesn't really explain what the PR is changing compared with the current behavior, or which existing cases were ambiguous or incorrect. Since the behavior around the first parameter, positional arguments, and ignore_equals has several interacting cases, it would help if you could summarize those changes.