Parse dimmaj7 as a diminished-major seventh, not a dim7 [patch] - #270
Merged
Merged
Conversation
DetermineSeventh tested the diminished-seventh branch before the major seventh. That branch asks only whether the body contains "dim" and a '7', which "dimmaj7" does, so the diminished-major seventh the formatter emits parsed back as a fully-diminished seventh. The round trip was broken and ChordTones placed the bb7 (9) where the major seventh (11) belongs, putting Voice() and playback a semitone out. The major-seventh check now runs first. "dim7" is unaffected: it carries no "maj". Fixes #269 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UscjStBJdW3uHm5NR289DX
|
This was referenced Sep 24, 2026
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.



Fixes #269
DetermineSeventhasked the diminished question first:That branch tests only for
"dim"and a'7'."dimmaj7"has both, so the diminished-major seventh — a real chord of the harmonic and melodic minor, and exactly whatAppendDiminishedemits forDiminished+Major— never reached thehasMaj7line. Swapping the two checks is the whole fix, as the issue proposes.Why the round-trip suite did not catch it
Worth calling out, because it changes where the test belongs.
ChordRoundTripTests.Corpusstarts from a symbol and assertsParse(s)equalsParse(Parse(s).ToString()). For"Cdimmaj7"that passes on the broken code: it parses to a dim7, formats as"Cdim7", and re-parses as a dim7 again. A stable round trip of the wrong chord. Adding the symbol to the corpus would have proved nothing.The chord object is the only honest starting point, since being the inverse of the formatter is the contract under test, so
DiminishedMajorSeventhRoundTripsbuildsnew Chord { Quality = Diminished, Seventh = Major }, asserts it formats as"Cdimmaj7", and asserts the re-parse comes back equal.Tests
Four cases:
ChordRoundTripTests.DiminishedMajorSeventhRoundTripsChordTests.Parse_DiminishedMajorSeventh_IsDiminishedTriadWithMajorSeventhChord.Parse("Cdimmaj7").SeventhisMajor, quality stillDiminishedChordTests.Parse_DiminishedSeventh_StillHasTheDiminishedSeventh"Cdim7"is untouched by the reorderChordTests.ChordTones_DiminishedMajorSeventh_HasTheMajorSeventhNotTheDoubleFlatSeventh[0, 3, 6, 11]rather than[0, 3, 6, 9]That last one is there because the misparse is not only a labelling error.
ChordTonesmapsSeventhType.Diminishedto offset 9 andMajorto 11, so a wrong parse is a semitone error inVoice()and in anything that plays the result — the silent half of the bug, and the half aSeventhassertion alone would not pin.Confirmed the tests depend on the change by restoring the original branch order and re-running: 3 failed, 1305 passed. The
"Cdim7"case passes either way by design — it is the guard on the reorder, not evidence for it.Full suite green on the fix: 1316 total, 0 failed, 1308 passed, 8 skipped (the skips are the pre-existing Windows-only path tests). Release build clean, zero warnings.
Not covered
hasMaj7isbody.Contains("maj") || body.Contains("M7") || body.Contains('Δ'), so a bare"Cmaj"with no digit already reportsSeventhType.Major. That is pre-existing and untouched here — the reorder can only change a body carrying both"dim"and a major-seventh spelling, which is"dimmaj7"and its variants."°maj7"was already correct, since the'°'spelling sets the quality without putting"dim"in the body, so the diminished branch never fired for it.🤖 Generated with Claude Code
https://claude.ai/code/session_01UscjStBJdW3uHm5NR289DX
Generated by Claude Code