RomanNumerals reject x/y/z in figures; use ⑪⑬⑮ as internal placeholders - #1998
Merged
Conversation
expandShortHand() collapsed the two-digit figures 11, 13, and 15 to the
single characters x, y, and z so that SHORTHAND_RE would find each as one
group. That leaked: RomanNumeral('Ix') parsed as I11, 'Iy' as I13, and
'Iz' as a nonsensical two-note C4 C4 chord, since x/y/z pass the
isalnum() figure validation.
The placeholders are now ⑪ ⑬ ⑮, which read as the numbers they stand for,
and a figure containing x, y, or z raises RomanNumeralException. No real
figure uses those letters: clercqTemperley rewrites its 'x' to 'o' before
constructing a RomanNumeral, and no corpus RomanText file has one.
Also strengthens the AGENTS.md / writing-docs rule that regression cases
belong in unittests, not docstrings.
AI-assisted (Claude)
|
coverage: 93.326%. remained the same — rn-xy into master |
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
2 participants
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.
A user on another site noticed that things like "IIIx" get interpreted as E-G#-B-D-F-A -- a III13 chord.
Why?
RomanNumeral.expandShortHand()collapsed the two-digit figures 11, 13, and 15 to the single characters x, y, and z so that SHORTHAND_RE would find each as one group. That leaked: RomanNumeral('Ix') parsed as I11, 'Iy' as I13 (and Iz as C4 C4!). Since x/y/z passed theisalnum()figure validation.The placeholders are now ⑪ ⑬ ⑮, which read in the code as the numbers they stand for, but they are turned back at the end. Creating a figure containing x, y, or z raises RomanNumeralException an construction (No real figure uses those letters: clercqTemperley rewrites its 'x' to 'o' before constructing a RomanNumeral, and no corpus RomanText file has one.)
Misc: Also strengthens the AGENTS.md / writing-docs rule that regression cases belong in unittests, not docstrings. -- my agent keeps wanting to document bugs and things not to do for all time! nope!
AI-assisted (Claude)