From 71fd9b8047b3542f6565144debf7b725afa18f99 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Mon, 10 Aug 2026 12:21:40 -1000 Subject: [PATCH] =?UTF-8?q?roman:=20reject=20x/y/z=20in=20figures;=20use?= =?UTF-8?q?=20=E2=91=AA=E2=91=AC=E2=91=AE=20as=20internal=20placeholders?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .agents/skills/writing-docs/SKILL.md | 6 ++++++ AGENTS.md | 6 ++++++ music21/roman.py | 21 ++++++++++++++------- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/.agents/skills/writing-docs/SKILL.md b/.agents/skills/writing-docs/SKILL.md index 6bc145c82..345b2b150 100644 --- a/.agents/skills/writing-docs/SKILL.md +++ b/.agents/skills/writing-docs/SKILL.md @@ -75,6 +75,12 @@ So: a fix for a crash on an edge case, a check that some input no longer produces invalid output, an assertion tied to an issue number — unittest. An example a user would want to read — doctest. +The trap is the one-line `Traceback` example proving that bad input now raises. +It looks like documentation and costs almost nothing to add, but the input comes +from the bug rather than from anything a user would write, so it teaches nothing +while making an obscure corner one of the first things a reader meets. It is a +unittest. + Naming the guarded bug **is** appropriate in a unittest; that is what the test is for. The rule against narrating old bugs applies to docstrings and to comments in shipping code, not to tests. diff --git a/AGENTS.md b/AGENTS.md index 0ebb83b5e..fed5f76f5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,12 @@ if on a single core machine.) - Run `uv run ruff check music21` before making PRs or pushes to open PRs. - Run `uv run mypy music21` before making PRs or pushes to open PRs. +- **Regression cases go in the module's `Test(unittest.TestCase)` class — never in a + docstring.** This is absolute, and it covers the tempting one-liner showing that some + bad input now raises. A doctest sits in the most-read documentation the project has, + so an example built from input no one would ever write teaches nothing and puts an + obscure bug on a billboard. The test: would a first-time reader of this object want + this example? If no, it is a unittest. See the `writing-docs` skill. - Never commit `forceSource=True` to a test or doctest (it re-parses from source every run and slows the suite for everyone). The ONLY exception is the one test that exercises `forceSource` itself. If you hit a stale-parse problem while developing: diff --git a/music21/roman.py b/music21/roman.py index 733261bd2..ca947d3d2 100644 --- a/music21/roman.py +++ b/music21/roman.py @@ -44,7 +44,7 @@ # ----------------------------------------------------------------------------- -SHORTHAND_RE = re.compile(r'#*-*b*o*[1-9xyz]') +SHORTHAND_RE = re.compile(r'#*-*b*o*[1-9⑪⑬⑮]') ENDWITHFLAT_RE = re.compile(r'[b\-]$') # cache all Key/Scale objects created or passed in; re-use @@ -232,18 +232,16 @@ def expandShortHand(shorthand): shorthand = shorthand.replace('/', '') # this line actually seems unnecessary. if ENDWITHFLAT_RE.match(shorthand): shorthand += '3' - shorthand = re.sub('11', 'x', shorthand) - shorthand = re.sub('13', 'y', shorthand) - shorthand = re.sub('15', 'z', shorthand) + # single characters for the two-digit figures, so that each group + # SHORTHAND_RE finds is one figure. + shorthand = shorthand.replace('11', '⑪').replace('13', '⑬').replace('15', '⑮') shorthandGroups = SHORTHAND_RE.findall(shorthand) if len(shorthandGroups) == 1 and shorthandGroups[0].endswith('3'): shorthandGroups = ['5', shorthandGroups[0]] shGroupOut = [] for sh in shorthandGroups: - sh = re.sub('x', '11', sh) - sh = re.sub('y', '13', sh) - sh = re.sub('z', '15', sh) + sh = sh.replace('⑪', '11').replace('⑬', '13').replace('⑮', '15') shGroupOut.append(sh) return shGroupOut @@ -2422,6 +2420,9 @@ def __init__( if not all(char.isalnum() or char in '#°+-/[]' for char in figure): # V, b, ø, no, etc. already covered by isalnum() raise RomanNumeralException(f'Invalid figure: {figure}') + if any(char in 'xyz' for char in figure): + # no roman numeral figure contains these letters + raise RomanNumeralException(f'Invalid figure: {figure}') # Store raw figure before calling setKeyOrScale: self._figure = figure @@ -4630,6 +4631,12 @@ def test_get_key_from_cache_mode(self): self.assertEqual(_getKeyFromCache('C').mode, 'major') self.assertEqual(_getKeyFromCache('c').mode, 'minor') + def testXYZAreNotFigures(self): + for fig in ('Ix', 'Iy', 'Iz', 'V7/ix'): + with self.subTest(figure=fig): + with self.assertRaises(RomanNumeralException): + RomanNumeral(fig, 'C') + class TestExternal(unittest.TestCase): show = True