[tacc-tacc]

@5j9 As we were discussing in the issues:

• rm_dup_args_safe and rm_first_of_dup_args shift positional arguments. Changed the implementation to forbid this breakage of semantics.
• Now the repeated tag is only added after checking it is not already present with endswith

Just for historic reasons I reverse iterated, but I think my new implementation of rm_dup_args_safe can work also with forward iteration. Feel free to add all the test you consider neccesary.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (756c510) to head (9879538).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #159   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         2358      2443   +85     
=========================================
+ Hits          2358      2443   +85     

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

[5j9]

def test_dont_change_arg1_value_from_x_to_y():
    t = Template('{{t|x|1=y|1=x}}')
    t.rm_dup_args_safe()
    assert t.string == '{{t|1=y|1=x}}'

currently it gets converted to {{t|x|1=y}} which is not ideal since the |1=y will override the first postional (x) and as a result the rendered value for arg 1 changes from x to y.

In this case {{t|1=y|1=x}} or {{t|1=y|x}} are more desirable I think.

[tacc-tacc]

@5j9 Perhaps checking instead if semantics will change after argument removal and not doing anything at all? What about {{t|x|y|1=y|1=x}}?

[5j9]

That would be fine if no better approach can be found. Although I think in my test case, {{t|1=y|1=x}} is preferable if feasible. Removing the first positional x does not change anything. Similarly, in your example, I think {{t|1=y|1=x}} is acceptable, though it might be a little more computationally costly/complicated.

We could also merge it as is and leave that test case for another issue.

[tacc-tacc]

@5j9 I modified the code to address both cases. I think it is only necessary to check when the current argument is positional, since it is the situation where are removed the previous arguments. Of course, semantics will be changed if the argument is empty, but that is how the function worked before. You may merge now and close the associated issues.

[5j9]

Merged into main locally after rebasing the PR branch. Closing manually since GitHub did not detect the rebased commits as a merge.