Properties use decorators throughout - #2012
Merged
Merged
Conversation
Roughly sixty properties across 25 modules move from the old `x = property(_getX, _setX, doc=...)` form to `@property` / `@x.setter`. The doc= text becomes the getter's docstring; internal callers of the private helpers (Stream.finalBarline recursing into parts, Measure's barline setters, Music21Object.purgeOrphans) now go through the property. Left as-is where property() is doing something a decorator cannot: - key.sharps: being handled on the sharps-int branch. - tempo.MetronomeMark .text/.number: the setters take an extra optional keyword, and _updateTextFromNumber() calls _setText with it. - scale .pitches/.chord: built from the public getPitches()/getChord(), which take arguments and are also called directly. - Stream.secondsMap: _getSecondsMap takes arguments. - braille/test.py: write-only property(fset=...). Annotating the getters made their types visible to mypy for the first time, which surfaced nineteen latent errors, all fixed here: - Stream.metadata is Metadata|None, so tsvConverter, clercqTemperley and humdrum.spineParser were dereferencing a possible None. Each now holds the Metadata it just inserted in a local. - mixedNumeral took numbers.Real, which mypy accepts for neither float nor Fraction; it now takes int|float|Fraction. - Metadata.fileNumber's setter claimed str while converter passes int|None. - meter.bestTimeSignature rebound the int `numerator` to a float mid-loop. - Duration.consolidate's currentTupletDuration needed OffsetQL, not float. - xmlToM21 assigned RepeatBracket.number from a MusicXML attribute that may be absent; None reached the setter and was caught by the except clause, which is now unnecessary for that case. 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
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.
short story - nearly all remaining
x = property(_getX, _setX, doc=...)become@property def x/@x.setter.Music21's been around for a while -- first supporting Python 2.2 which defined properties as:
Since all the way since Python 2.4 it's been possible to define properties as:
so when 2.4 became m21 minimum we started slowly using the second form. But music21 also uses doctests extensively. The problem was that while the old-style
x = property(_get, _set)reported line numbers of failing doctests properly, the new-style@propertydid not. So debugging music21 was much harder with the new version. I pushed hard for a fix which finally as a Christmas 2023 present Serhiy Storchaka pushed through got a simpler solution into Python 3.13 (backported to 3.12.3 and 3.11.9) so it's finally available on basically all Python systems music21 runs on.But like sitting asbestos, there was no need harm from old code so stirring up the pot and changing properties that already worked in music21 wasn't a priority. (Plus the new code is inelegant...note below.)
But in the past year or so, two things have changed:
property()So it's worth a change. 60ish properties across 25 modules are refactored. Left along some places where the
_get()or_set()form does double duty (such as having a second argument), and braille test's cool write-only properties.Agent found 19 possible errors once mypy got their types. Mostly harmless (like setting
stream.metadata = Metadata(); stream.metadata.title = ...getting astream.metadata might be None) or type-only problems. (We did well there!)AI-assisted (Claude)
Property ugliness
Not that anyone cares what I think, and probably too late/ship was sailed, unless we use a new term like
@propbut the duplication of the function name in the setter is so ugly. Why not instead ofjust do this:
And have in the basic case that one-argument = getter, and two-arguments = setter, and lookup is by matching name in the parsing. (The same type of magic as allowing
super().__init__()instead ofsuper(self, Duration).__init()in Py3)Yes, there are some cases where you want to define just the setter alone or the thing you are setting has a different name etc. but there's a wonderful
property()function for those strange cases!