Repository navigation
Fix mfenced separators in canonicalization - #895
Merged
Merged
Conversation
convert_mfenced_to_mrow did not follow the MathML rules for the separators attribute: - separators="" inserted a comma between all children instead of no separators - with too few separators a comma was used instead of repeating the last separator - whitespace in separators was used as a separator instead of being ignored Each case has a unit test. The expected output of mfenced_with_separators is corrected to repeat the last separator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Linux library size: 0.59 MiB (+0.06%)
Built with default features, Rust 1.96.0, and Ubuntu 24.04. Workflow run. |
Small tweak for more efficient code/more idiomatic code: `.or(separators.last())` => `or_else(|| separators.last())` This avoids evaluating `separators.last()` when `separators.get(i)` is already `Some`.
Collaborator
|
Thanks for spotting the bug and providing a fix. I tweaked the fix, but it is basically what you provided. |
Contributor
Author
|
Thank you for the refactor, that helps me to learn more too. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
convert_mfenced_to_mrowinsrc/canonicalize.rsdid not follow the MathML 3 rules for theseparatorsattribute ofmfenced, as described in section 3.3.8.2 Attributes. This PR fixes three cases. Each case has a small unit test.1. Too few separators
Earlier, MathCAT used a comma as the fallback separator. Perhaps the confusion arose from what the spec says next:
The existing test
mfenced_with_separatorsexpected the old behavior, so its golden was changed.2. Empty separators
Again, in this case the comma was inserted as default/fallback. Dedicon's braille rules ran into this case (
mfenced_empty_separators), which admittedly is an edge case, but does occur in production.Note that if the
separatorsattribute is not provided, its default is the comma. We test this too.3. Whitespace in separators
Whitespace is now stripped. This has some interesting relations with other rules, e.g.
separators=" "yields no separators, because whitespace is ignored.