KeySignature.sharps is always an int. isNonTraditional now settable - #2011
Merged
Conversation
…attribute `sharps` was `int|None`, with `None` meaning "non-traditional key signature." That made every caller either handle a None it would never see or paper over it (`self.sharps or 0` in asKey), and it left `isNonTraditional` as a derived read-only property. Now `sharps` is always an int and `isNonTraditional` is a plain bool attribute that users set directly. The property was declared as `sharps = property(_getSharps, _setSharps, ...)`, which mypy cannot see through -- `ks.sharps` revealed as `Any`, so annotating the getter alone bought nothing. Converted to the decorator form, and `reveal_type(ks.sharps)` is now `int`. `KeySignature(sharps=None)` still works for this cycle: it warns Music21DeprecationWarning and sets sharps=0, isNonTraditional=True. mypy flags it at the call site, which is the intended pressure during the deprecation. Only MusicXML supports non-traditional key signatures; MEI, ABC, humdrum, capella, musedata, noteworthy and romanText all build from an int. Annotating nonTraditionalKeySignature surfaced three latent errors in its body, where Element.text is str|None: an unannotated accidentals list, float(c.text), and a list[Pitch] assigned through the invariant alteredPitches setter (that setter now takes an Iterable). Also drops the unreachable "sharps None" braille message and the None fixup in sharpsToPitch. Version bumped for the pickle cache: the cache is version-keyed, and a master checkout reading b8 pickles written by this code fails 123 tests with "property 'isNonTraditional' of 'Key' object has no setter". Separately, fixes music21/duration.py's TupletFixer.fixBrokenTupletDuration docstring, which set humdrum.spineParser.flavors['JRP'] = True and never restored it. `flavors` is a module-level dict, so the leak was global and permanent for the process: under pytest's alphabetical order duration.py runs before humdrum/tests.py, and testSingleNote then parsed `40..` in JRP flavor, putting the two dots on the note (dots == 2) instead of the tuplet's durationNormal (dots == 0). CI never saw it because testSingleCoreAll sorts modules by mtime, and on a fresh clone all mtimes tie and it falls back to reverse-alphabetical, running humdrum before duration -- but any PR touching duration.py would have pushed it to the front and tripped the failure. AI-assisted (Claude)
The CI runner (testSingleCoreAll) imports every module through ModuleGather under its short name, so the class repr is `<key.KeySignature ...>` there and `<music21.key.KeySignature ...>` under pytest. Assert only the description. AI-assisted (Claude)
ModuleGather.getModule loaded each file with load_source() under its bare name
('key' for music21/key.py), which re-executed the file as a separate top-level
module. Every unittest the single-core runner ran therefore tested a shadow
copy of its module: `mod.KeySignature is not music21.key.KeySignature`, and
`isinstance(ks, music21.key.KeySignature)` was False. It also gave those
classes a `__module__` of 'key', so `repr()` came out as
`<key.KeySignature of 2 sharps>` and any test asserting a full repr failed
under CI while passing under pytest.
Import by fully-qualified name instead, which returns the module music21
already imported. load_source() is now unused and removed.
multiprocessTest was unaffected -- it uses getModuleWithoutImp, which walks the
real package tree.
Restores the exact repr assertion in key.Test.testNonTraditional.
AI-assisted (Claude)
|
coverage: 93.331% (+0.002%) from 93.329% — sharps-int into master |
The runner fix does not belong in a KeySignature PR. Reverts commonTest.py to master and instead follows the convention used elsewhere: import the module inside the test method, so the test uses music21.key.KeySignature and its repr is fully qualified under both runners. AI-assisted (Claude)
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.
KeySignature.sharps (and thus Key.sharps) is now always an int, so systems can safely add/subtract, etc. them.
The previous way of specifying a non-traditional key signature (sharps=None) was from before the days where there were big advantages to consistent and simple typing. But we love our Bartók and music of the rest of the world, so now
isNonTraditionalis plain bool. Still supported in MusicXML.Fix unrelated Josquin/Humdrum bug that arose when running tests in different order. Fixed small bugs in MusicXML parsing that better typing surfaced.
AI-assisted (Claude)