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

Add warning when using <constraintSpec> inside <classSpec> #2173

Open
HelenaSabel opened this issue Aug 20, 2021 · 7 comments
Open

Add warning when using <constraintSpec> inside <classSpec> #2173

HelenaSabel opened this issue Aug 20, 2021 · 7 comments

Comments

@HelenaSabel
Copy link
Member

As a temporary solution for Stylesheets ticket #519, a warning should be fired whenever a <constraintSpec> is used inside <classSpec> without an explicit @context.

@sydb
Copy link
Member

sydb commented Aug 21, 2021

Actually, since <constraintSpec> is permitted in lots of (mostly ridiculous) places,[1] methinks the warning should be fired whenever a <constraintSpec> is used without an explicit @context anywhere an intelligent context is not generated by the Stylesheets.

So the first step in resolving this ticket is generating the list of places the Stylesheets do provide an intelligent context. Schematron is processed in at least 2 different ways, I submit that if the context is generate in either method, we should not issue a warning.

[1] The list is: <ab>, <accMat>, <acquisition>, <add>, <additions>, <argument>, <attDef>, <body>, <camera>, <caption>, <case>, <castList>, <cell>, <change>, <classSpec>, <collation>, <colloc>, <condition>, <corr>, <custEvent>, <damage>, <dataSpec>, <decoNote>, <def>, <del>, <desc>, <dictScrap>, <div>, <div1>, <div2>, <div3>, <div4>, <div5>, <div6>, <div7>, <docEdition>, <elementSpec>, <emph>, <entryFree>, <epigraph>, <epilogue>, <etym>, <fDescr>, <figDesc>, <figure>, <filiation>, <foliation>, <form>, <fsDescr>, <gen>, <gram>, <gramGrp>, <handNote>, <head>, <hi>, <hyph>, <iType>, <imprimatur>, <item>, <l>, <lang>, <layout>, <lbl>, <lem>, <licence>, <macroSpec>, <meeting>, <metamark>, <mod>, <mood>, <musicNotation>, <note>, <number>, <occupation>, <orig>, <origin>, <orth>, <p>, <per>, <performance>, <pos>, <postscript>, <prologue>, <pron>, <provenance>, <q>, <quote>, <rdg>, <ref>, <reg>, <rendition>, <restore>, <retrace>, <rhyme>, <said>, <salute>, <schemaSpec>, <scriptNote>, <secl>, <seg>, <set>, <sic>, <signatures>, <signed>, <sound>, <source>, <specGrp>, <stage>, <stress>, <subc>, <summary>, <supplied>, <support>, <surplus>, <surrogates>, <syll>, <tagUsage>, <tech>, <title>, <titlePart>, <tns>, <trailer>, <typeNote>, <u>, <unclear>, <usg>, <view>, <witness>, <writing>, and <xr>. Aren’t you sorry you asked?

@sydb sydb self-assigned this Aug 21, 2021
@sydb sydb added this to the Guidelines 4.3.0 milestone Aug 21, 2021
@sydb
Copy link
Member

sydb commented Aug 21, 2021

Contexts in which odds/teiodds.xsl generates a @context
  • attDef//
  • elementSpec//
Contexts in which odds/extract-isosch.xsl generates a @context
  • elementSpec//attDef//
  • elementSpec//
  • schemaSpec//, but not one of above (I think this should just be schemaSpec/)

Note that it seems like @validUntil generates proper @context for deprecation when it is on at least macroSpec, elementSpec//attDef, classSpec//attDef, elementSpec, elementSpec//valItem, and macroSpec.

Contexts in which odds/extract-isosch.xsl generates a poor-man’s @context and issues a warning
  • classSpec//attDef//
  • classSpec//

These should still be invalid, IMHO. (That is, I think the TEI schema should still say to a user “don’t use Schematron without a context in a <classSpec>, even if the processor would also issue a warning.)

Contexts in which odds/extract-sch.xsl generates a proper @context

I don’t think we care, because I don’t think this program is used anymore at all.

result

So it seems to me that

<rule context="constraintSpec[@scheme eq 'schematron' and not( .//@context )]">
  <assert test="ancestor::elementSpec | ancestor::attDef | parent::schemaSpec"> WARN </assert>
</rule>

would almost do the job. Only thing is, have to think about <specGrpRef>s, which add another (large) layer of complexity.

@martindholmes
Copy link
Contributor

It seems to me that allowing constraintSpec anywhere except tagdocs elements makes no sense at all. In the case of elementSpec/attDef, IIRC the context that is generated is often wrong; I'll test that out and report back.

But when you say elementSpec//, do you mean any descendant of elementSpec? That seems wrong. I would say (in all cases) direct child only.

@sydb
Copy link
Member

sydb commented Aug 23, 2021

Well, yes and no. I did mean “descendant of <elementSpec>”, because that is what the Stylesheets say. While I would be more than happy to go about restricting where <constraintSpec> can go (elementSpec/listRef/ref/constraintSpec; really??), that is another ticket.

There is, of course, at least one sensible place inside <elementSpec> for <constraintSpec> to go other than as direct child: inside an <attDef>. Because a context is automaticaly generated in that case, it should not (IMHO) get a “you did not use a context” warning.

In the specific case of elementSpec//attDef (which is what I presume you mean), I do not know what odds/teiodds.xsl does. But odds/extract-isosch.xsl does

          <xsl:value-of select="ancestor::elementSpec/@nsp"/>
          <xsl:value-of select="ancestor::elementSpec/@ident"/>
          <xsl:text>/@</xsl:text>
          <xsl:value-of select="ancestor::attDef/@nsp"/>
          <xsl:value-of select="ancestor::attDef/@ident"/>

which looks perfectly correct to me. (By this pass the @nsp attribute has been placed an all the spec elements.) Probably better expressed as

          <xsl:sequence select="
              ancestor::elementSpec/@nsp
            ||ancestor::elementSpec/@ident
            ||'/@'
            ||ancestor::attDef/@nsp
            ||ancestor::attDef/@ident"/>

these days.

@martinascholger martinascholger removed this from the Guidelines 4.3.0 milestone Aug 31, 2021
@sydb
Copy link
Member

sydb commented Sep 13, 2021

OK, I think I now have a working solution. As expected, the Schematron to test this ignoring <specGrp> and <specGrpRef> is all but trivial; the Schematron to test this taking account of (only local) <specGrp> and <specGrpRef> is quite complex. (Perhaps someone else can simplify it.)

In any case, this needs testing, and it is not easy to test. While I have created a single ODD file that both contains the proposed constraint and has test cases for it, you can’t just generate schemas from it and validate against them because, by definition, the Schematron schemas generated from some of the test cases have errors. (That’s why we want a constraint to catch such problems at validation stage!)

So I have also created a test Schematron file by extracting the useful bits out of the generated .isosch file.

So it seems to me the procedure for testing this is to do at least the following:

  1. download the ODD and Schematron file (which are together in issue_2173_2023-02-01T13.zip.)
  2. read the ODD file (either directly or generating HTML or whatever if you prefer)
  3. scan through the extracted Schematron file enough to convince yourself it is what it claims to be
  4. validate the ODD file against the Schematron file
  5. Hope you get only the expected messages

And we need better wording of the warning messages themselves, of course.

If that all goes OK (including someone coming up with better messages), I will generate a pull request with the new constraint (in Specs/constraintSpec.xml, presumably).

@sydb
Copy link
Member

sydb commented Feb 1, 2023

Note — I have edited the previous post by putting a new test file. The main difference is that the old test file was missing the word “not” in a key bit of prose which could have lead to serious confusion. 😕

@martindholmes martindholmes added the atop another TEI ODD processor label May 31, 2023
@sydb
Copy link
Member

sydb commented Jun 9, 2023

Council makes GREEN to implement, with the further note that we are seriously considering requiring <sch:rule> with @context always. (Which would make this ticket obsolete.)

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

No branches or pull requests

5 participants