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

no duplicate <attDef>s allowed #2534

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from
Open

no duplicate <attDef>s allowed #2534

wants to merge 17 commits into from

Conversation

sydb
Copy link
Member

@sydb sydb commented Mar 14, 2024

Address #2371 with a Schematron constraint that flags as an error any 2+ <attDef>s that both share an ancestor <attList> and have the same @ident (regardless of the @mode) unless they are both (all) children of an <attList> that has an @org attribute value of "choice".

The file detest.odd has been updated to test this reasonably well.

If tests pass, I personally do not think any deprecation period is necessary. (What would it deprecate, anyway?) The error this new constraint catches is an error.

Reviewers: If you actually look at the code differences the most important bit (by far) to look at is the new lines 24–76 (in green) of attList.xml.

that flags as an error any 2+ attDef elements who both share an ancestor attList and
have the same ident= attribute (regardless of the mode= attr value) unless they are
both (all) children of an attList that has an org= attribute value of "choice".
@martindholmes
Copy link
Contributor

@sydb Does this take account of namespaces? You could correctly have two atts with the same @ident but in different namespaces, no? Forgive me if I've missed it; I'm reading on my phone.

@sydb
Copy link
Member Author

sydb commented Mar 15, 2024

No, it does not. For some reason I guess I thought we had decided to look at only @ident (not +@ns), but that would be silly.
Will try to fix v. soon.

@sydb sydb marked this pull request as draft March 15, 2024 20:25
Per suggestion @martindholmes, make sure to think of two attributes that have the same local
name (i.e., ident= attribute) but different namespaces (i.e., ns= attribute); since the
attList element does not have an ns= attribute, we do not have to worry about inheritance,
which makes this much easier.
@sydb sydb marked this pull request as ready for review March 15, 2024 21:53
@sydb
Copy link
Member Author

sydb commented Mar 15, 2024

Fixed, and added more tests to detest.

@sydb
Copy link
Member Author

sydb commented Mar 15, 2024

One possible concern is that the comments I put in the Schematron show up in the Guidelines. Some folks will like that, others won’t.

@martindholmes
Copy link
Contributor

@sydb I know you will think this is trivial, but I don't think we should use YouTube URLs as spurious namespaces in tests. I don't think it's polite to commandeer someone else's domain for this sort of purpose, and I would also be concerned that any controversy or conflict arising in relation to one of these URLs in future might draw the TEI into something it has no connection with. I would prefer to use something like http://www.tei-c.org.ns/test/eg1, 2, 3 and so on. If you have no strong preference for keeping these YouTube URLs, I'll happily add a fix to the PR for this.

that flags as an error any 2+ attDef elements who both share an ancestor attList and
have the same ident= attribute (regardless of the mode= attr value) unless they are
both (all) children of an attList that has an org= attribute value of "choice".
Per suggestion @martindholmes, make sure to think of two attributes that have the same local
name (i.e., ident= attribute) but different namespaces (i.e., ns= attribute); since the
attList element does not have an ns= attribute, we do not have to worry about inheritance,
which makes this much easier.
Per suggestion @ebeshero use a different host for easter egg namespace URIs. Also required update to expected results to match new formatting of Schematron msgs. (Not sure why that change wasn’t already present.)
@sydb
Copy link
Member Author

sydb commented Apr 13, 2024

  1. Rebased current dev into this branch
  2. Addressed concern about namespace URIs, above
  3. Tested, passes all tests on my local system (except NVDL and XeLaTeX do not work on my system, so are untested). Only error is the 3 <superEntry> warnings that have nothing to do with this PR.

@ebeshero
Copy link
Member

ebeshero commented Apr 13, 2024

@martindholmes Our VF2F subgroup agreed, and our suggestion was just to make up a portmanteau URL, likehttps://teitube.org or some such. Re-reading now, your ns suggestion is probably more befitting to the task.

that flags as an error any 2+ attDef elements who both share an ancestor attList and
have the same ident= attribute (regardless of the mode= attr value) unless they are
both (all) children of an attList that has an org= attribute value of "choice".
sydb added 2 commits May 16, 2024 20:11
Missing namespace prefix in context attribute added, and results to match
description and some comments of new no_duplicate_attrs constraint; change
attr names to reflect NameSpace + IDENTifier combination
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.

Apologies for being slow on this. I keep looking at the Schematron rule and realising that I don't quite understand it, and I need to go away and write some specific tests. I don't get what the notanamespace thing is doing.

@sydb
Copy link
Member Author

sydb commented Jun 26, 2024

I think the reason $notanamespace exists is because I wanted to compare @ns eq $ad/@ns. I was either worried that it was not legal to perform the comparison if one or both did not exist, or I was worried that if @ns is "ab" and @ident is "br", then just comparing @ns||@ident would erroneously succeed when $ad/@ns was missing and $ad/@ident is "abbr".

Either way, though, on reading it now, since the comparisons are done separately (not combined as in para above), we might be able to get away without the $notanamespace.

But I tried removing the $notanamespace bit and just comparing @ns eq $ad/@ns, and the tests fail because in "no_duplicate_attrs_2_valid" the attribute definition for @fun is incorrectly flagged as a duplicate. (It is not a duplicate because it is in an alternation, i.e. its parent <attList> has org="choice".)

If I were actually writing XSLT I would probably do this in multiple steps. First take $defs (the sequence of all <attDef> descendants) and generate a sequence that, for each one, is the namespace & ident in Q-notation. Then a) all comparisons are just simple string comparisons and b) it is easy to generate Q-notation output. But I am not sure that is reasonable to do in Schematron alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants