You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This should resolve some of the noted discrepancies in the FITB schema.
bare-text in variable is allowed in the schema and is treated internally by automatically wrapping inside de-number. This should preserve the old treatment while ensuring that the text content is escaped inside a string as needed and parsed in javascript.
de-evaluate is now also allowed inside the variable (would have always worked but was never parsed) to allow for nested evaluation, such as would be used for evaluation of a substitution followed by composition.
I attempted (with Claude assistance help) to update the schema files to match this work. (separate commit)
Reviewed at cfefa4e0. All three schema files are the dev/experimental schema — de-number is absent from the stable pretext.rnc — which lowers the stakes on the reduced-behavior change below.
Three changes here. Two are complete; one is half-done.
de-evaluate inside variable — complete
The schema adds DEEvaluate to the variable content model, the XSL adds it to the apply-templates select and the guard, and the description explains it. Schema, code, and description agree, and the derived .rng matches the .rnc. No concern.
de-number@reduce default — deliberate, but a silent change to generated output
The old de-number always emitted .reduce().simplifyConstants(); the new code gates it on @reduce="yes". This is really a consistency fix: de-expression (all three modes) and de-evaluate already gate reduction on @reduce="yes", so de-number was the lone element that always reduced.
Building the sample article before and after this change, exactly the three bare de-number elements shift:
Everything carrying reduce="yes" (and every de-expression) is untouched, and the committed content is already annotated with reduce="yes" exactly where reduction is wanted. So the change is intentional and self-consistent. Since the feature is dev-only, no announcement is owed — just flagging that it does change generated JS for any bare de-number.
Bare text in variable — the XSL and the schema disagree
The description says bare text in variable "is allowed in the schema and is treated internally by automatically wrapping inside de-number." The XSL half is here — the variable otherwise-branch routes bare text through create-de-number. But the schema content model is still (Eval | DENumber | DEExpression | DEEvaluate) with no text, so bare text still fails validation:
error: text not allowed here; expected element "de-evaluate", "de-expression", "de-number" or "eval"
(built -f assembly-version, checked against pretext-dev.rng). So that otherwise-branch is unreachable for any valid document, and the updated schema comment ("plain-text values ... automatically wrapped in de-number") describes behavior the grammar forbids. If bare text is meant to be valid, text needs to be added to DEVariableValued in schema/pretext.xml and the schema regenerated; otherwise the branch is dead code and the description overstates the change.
The message id fields are empty — @visible-id is never an attribute
Both new messages report an exercise id, but neither resolves:
line 390: <xsl:value-of select="../@visible-id"/>
line 419: <xsl:value-of select="ancestor::*[@visible-id][1]/@visible-id"/>
visible-id is not a stored attribute anywhere — it is produced by the getter template <xsl:template match="*" mode="visible-id"> (pretext-common.xsl:3723), which returns @unique-id. An assembled tree carries a @unique-id on every element (~15.5k in the sample article) and zero @visible-id, and nothing in the XSL ever writes a visible-id attribute. So every bare @visible-id read here evaluates to the empty string, and both messages print ... in "". This is not new to the PR — the same file already has seven such reads (lines 86, 411, 464, 469, 542, 899, 949) — but the two new ones add to it.
The correct form is the getter:
<xsl:apply-templatesselect="."mode="visible-id"/>
on the context node whose id you want (the enclosing exercise). @rbeezer knows the whole id situation has been a mess, but retrieving these values through templates rather than bare attribute reads gives one seam to rationalize it behind some day. The two new messages should switch to the getter within this PR's changes. The seven pre-existing sites in this file are a separate matter — those can be swept whenever convenient, in their own change.
The diff is otherwise clean — no trailing whitespace, .rng regenerated to match .rnc, and PTX:FALLBACK/PTX:WARNING are both established prefixes.
Claude Opus 4.8, acting as a review assistant for Rob Beezer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should resolve some of the noted discrepancies in the FITB schema.
variableis allowed in the schema and is treated internally by automatically wrapping insidede-number. This should preserve the old treatment while ensuring that the text content is escaped inside a string as needed and parsed in javascript.de-evaluateis now also allowed inside thevariable(would have always worked but was never parsed) to allow for nested evaluation, such as would be used for evaluation of a substitution followed by composition.I attempted (with Claude assistance help) to update the schema files to match this work. (separate commit)