Skip to content

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

Open
tacc-tacc wants to merge 1 commit into
5j9:mainfrom
tacc-tacc:main
Open

fixed template and parser function "=" semantic#150
tacc-tacc wants to merge 1 commit into
5j9:mainfrom
tacc-tacc:main

Conversation

@tacc-tacc

Copy link
Copy Markdown
Contributor

@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 Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         2270      2326   +56     
=========================================
+ Hits          2270      2326   +56     

☔ 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

Copy link
Copy Markdown
Contributor Author

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

@tacc-tacc
tacc-tacc force-pushed the main branch 2 times, most recently from efc9d63 to dd29fb1 Compare September 4, 2026 07:11
@tacc-tacc

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

5j9 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

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:

@tacc-tacc

Copy link
Copy Markdown
Contributor Author

@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 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

@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

5j9 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants