make "spec-in-module" test less confusing

[sydb]

Replace the <constraint> "spec-in-module" with that which was recommended on issue #2898.

[jamescummings]

I have not tested this rule, but the XPath looks good to me. (And the logic behind it seems to make sense, though I did have to read the very helpful note three times.)

[martindholmes]

@sydb If I'm understanding correctly, the last test is the one most likely to fire for most well-formed customizations:

( //tei:moduleSpec/@ident | //tei:moduleRef/@key ) = current()/@module

so wouldn't it make sense to put that one first, to avoid having to evaluate the others? (I'm assuming there's no intervening optimization taking place somewhere.)

[sydb]

Short answer, @martindholmes, is I have no idea. That is, no idea which way might be more efficient. But in my brain it is easier to figure it out if the steps are “First, see if it even makes sense to test; then, if so, do this test”. In truth, I think of rule/@context as setting up whether the test should be fired or not, so to me it probably should be

<sch:rule context="( tei:elementSpec[@module] | tei:classSpec[@module] | tei:macroSpec[@module] )[
                     ( ancestor::tei:schemaSpec | ancestor::tei:TEI | ancestor::tei:teiCorpus )
                     and
                     ( //tei:moduleSpec | //tei:moduleRef ) ]">
  <sch:assert test="( //tei:moduleSpec/@ident | //tei:moduleRef/@key ) = current()/@module">
    Specification <sch:value-of select="@ident"/>: the value of the module attribute
    ("<sch:value-of select="@module"/>") should correspond to an existing module, via a
    moduleSpec or moduleRef</sch:assert>
</sch:rule>

That is, put all the conditions in rule/@context and only the test in assert/@test (or report/@test). But I thought other folks might balk at that complexity being in @context, especially since we have had all the complexity in @test for well over a decade, now.
But if you think that is as easy or easier for a human to follow, I would be happy to change.

P.S. I think it even makes the explanatory comment simpler, but not sure, I have not written it yet. 😄

[sydb]

I have now re-written to the new “all the logic about whether we should be looking to see if the module is defined or not in the @context” method. I like this better in large part because the comment is easier process. Or at least I think so. @jamescummings?

[martindholmes]

Just a quibble, but could I ask for a period at the end of the sch:assert? We haven't been consistent in this in the spec files, but we should be. If it's a full sentence, it should end with a period. I should do a trawl through the rest of our Schematron and standardize this.

[sydb]

… could I ask for a period at the end of the sch:assert? …

Done.
See also #2905.

[martindholmes]

Looks good!

[ebeshero]

@sydb I think by now we have most of us agreeing with the logic and syntax, but I am wondering if we actually tested it on an actual ODD in the wild? I think I could do so this afternoon and if it works, just merge this...

[sydb]

Good question, @ebeshero. I do not know if I have ever seen this error either before or after my change. Hang on, I will try testing it (both in dev branch and this branch) against all the ODDs in the ATOP collection …

[sydb]

(You wanna a build a test ODD file that should fail this test?)

[ebeshero]

Yes--that's what I was thinking... Also, I've just been testing the branch and so far all's well there.

[sydb]

Ha!
There are ~61 ODD files in the ATOP hierarchy … I tested all of them. In those 61 files there are 14 errors flagged by this constraint (the "spec-in-module" constraint) in 5 files. All 28 messages are different … after this change, they end in a period! 😄
So it looks to me like it works exactly as expected. (In all 14 cases the XPath flagged as an error is the same before & after this change, the only difference is the ultimate period.)

[ebeshero]

Well! On the strength of that beautiful testing, I'm just going ahead and merge this branch! :-D