Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p5odds.odd bug fix #2629

Merged
merged 2 commits into from
Dec 4, 2024
Merged

p5odds.odd bug fix #2629

merged 2 commits into from
Dec 4, 2024

Conversation

sydb
Copy link
Member

@sydb sydb commented Dec 4, 2024

The constraint on <eg> was looking for all descendant text nodes, not just child text nodes. That means that if there is any phrase-level encoding in an <eg> then there is more than one text node that has (for example) position()=last(), and the XPath fails because only one item is allowed as the first argument to matches().

Fix seems trivial: search only for child text nodes, not descendant text nodes.

Note: I am not sure the “TEI: ODD” label is correctly — it might be for the TEI language defined by the Guidelines, not the TEI language defined for the Guidelines. Sigh. Anyway, this is high priority because it is holding up work on #2516.

Constraint was looking for _all_ descendant text nodes of eg element,
not just child text nodes. That means that if there is any phrase-level
encoding in an eg element there is more than one text node that has (for
example) position()=last(), and the XPath fails because only one item is
allowed as the first argument to matches().
Per suggestion of @martindholmes, instead of trying to select the correct
descendant text node(s), just examine the string value of the entire eg
element as a whole. The string() function call is not technically necessary
(because matches() wants a string there, it would happen automagically),
but its use makes what we are doing explicit and obvious.
@sydb
Copy link
Member Author

sydb commented Dec 4, 2024

Correction to OP: @martindholmes suggests that we, instead of attempting to get the right text() node to test, just test the entire string value of the <eg>. I think he is right, so have switched test to use string(.) instead. (Strictly speaking the call to string() is not necessary, as matches() is expecting a string as its 1st argument and thus will automagically convert, but calling the function makes what we are doing explicit.)

@sydb sydb requested a review from martindholmes December 4, 2024 17:06
@sydb sydb self-assigned this Dec 4, 2024
Copy link
Contributor

@martindholmes martindholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

@sydb sydb merged commit cb8bad5 into dev Dec 4, 2024
2 checks passed
sydb added a commit that referenced this pull request Dec 4, 2024
The bug addressed by #2629 prevents this from building. Committing now so
I can apply that bug-fix here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants