XML minimal diff saving bug with text markup

[nschneid]

closes #9324

This lists the markup fields in TAGS_WITH_MARKUP, which seems to do the trick. I don't fully understand why this matters though...

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.89%. Comparing base (5e8c8d6) to head (4d40dd4).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9326   +/-   ##
=======================================
  Coverage   96.89%   96.89%           
=======================================
  Files          35       35           
  Lines        3702     3704    +2     
=======================================
+ Hits         3587     3589    +2     
  Misses        115      115           

Files with missing lines	Coverage Δ
python/acl_anthology/utils/xml.py	96.58% <100.00%> (+0.05%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Build successful. Some useful links:

• Complete site preview: https://preview.aclanthology.org/xml-markup-diff-fix
• Potential changes of interest:

This preview will be removed when the branch is merged.

[mbollmann]

Two of the tags don't belong here, the rest must be added, yes.

[mbollmann]

Okay, if I remove tex-math from TAGS_WITH_MARKUP, this test fails:

    (
        "<abstract><tex-math>a</tex-math> <b>xxx</b></abstract>",
        "<abstract><tex-math>a</tex-math> xxx</abstract>",
        "<abstract><tex-math>a</tex-math> <b>xxx</b></abstract>",
    ),

Pretty wild to me that this wasn’t caught earlier. In any case, adding this to TAGS_WITH_MARKUP is only accidentally doing the right thing here and shouldn't be correct. I will investigate.

[mbollmann]

I think the solution is to just stop the minimal-diff algorithm from recursing further into markup-containing tags. Strange that (i) this wasn’t how it already worked, and (ii) we haven’t encountered a situation triggering a bug from this earlier.

[nschneid]

I was confused by TAGS_WITH_MARKUP as it mixes what are two distinct categories in my mind: (a) markup tag names like "b" and (b) MarkupText-valued publication attributes like "abstract".

It would be great if the list could be limited to category (b) and that was sufficient to block recursing into the MarkupText when computing the minimal diff.

[nschneid]

Also worth noting that the list of MarkupText-valued attributes could be extended in the future e.g. for awards (#9101).

[mbollmann]

I was confused by TAGS_WITH_MARKUP as it mixes what are two distinct categories in my mind: (a) markup tag names like "b" and (b) MarkupText-valued publication attributes like "abstract".

It would be great if the list could be limited to category (b) and that was sufficient to block recursing into the MarkupText when computing the minimal diff.

It’s not just for the minimal-diff, but also for indentation. I don’t think there is any mixing of categories here: TAGS_WITH_MARKUP lists all the tags which may contain MarkupText according to the RelaxNG schema. It’s a one-to-one correspondence.

[mbollmann]

Incidentally, that’s why tex-math doesn’t belong there: It may appear within a MarkupText block, but it may not itself contain any MarkupText. The semantics of TAGS_WITH_MARKUP is the latter.

[nschneid]

How is it used for indentation?

[nschneid]

Incidentally, that’s why tex-math doesn’t belong there: It may appear within a MarkupText block, but it may not itself contain any MarkupText. The semantics of TAGS_WITH_MARKUP is the latter.

Then <url> should also be excluded? The schema restricts it to URIs.

[mbollmann]

Then <url> should also be excluded? The schema restricts it to URIs.

Yes, that's what I commented above and why I already removed it again.

[mbollmann]

How is it used for indentation?

From a quick look at the code: Markup-containing tags are not recursed into and will not get indentation after them if they have children. It may be that for this purpose, listing the "high-level" markup tags suffices, but I still contend that the semantics of TAGS_WITH_MARKUP is unambiguous and clearly documented.

[nschneid]

@mbollmann What's the next step here?

[mbollmann]

@mbollmann What's the next step here?

Merging. :)

Since I pushed my own changes here I’d find it weird to self-approve, but @mjpost has done this now, so I think we’re good.

[nschneid]

Oh, hmm, I want to try some more test cases as I'm not sure:

• Will recursing be blocked for <url> and <tex-math>?
• Does <par/> belong in TAGS_WITH_MARKUP? It doesn't contain any text.

[mbollmann]

• Will recursing be blocked for <url> and <tex-math>?

They can never contain any children, as per the schema, so by their nature they can’t be recursed into.

• Does <par/> belong in TAGS_WITH_MARKUP? It doesn't contain any text.

Oh, that’s a good point, by the semantics I have given it indeed probably doesn’t belong there.

[nschneid]

Also, I just realized that if titles and abstracts aren't being recursed into, then by definition any markup isn't being recursed into.

[nschneid]

(cannot merge because changes were requested)

[mjpost]

I assume you addressed his comments? I removed him as a reviewer, not sure why it's blocked

Addressed

[nschneid]

Hmm I had to explicitly dismiss the review. That was confusing