diff --git a/.agents/skills/bump-version/SKILL.md b/.agents/skills/bump-version/SKILL.md index 31c0320d0..40a08b891 100644 --- a/.agents/skills/bump-version/SKILL.md +++ b/.agents/skills/bump-version/SKILL.md @@ -35,27 +35,37 @@ uv run pytest --doctest-modules music21/base.py music21/_version.py ## When to bump -- **Parsing-format / parser changes (most common reason).** Any change to how a - format is read or written — musicxml, abc, MIDI, noteworthy/NWC, humdrum, - etc. — should bump the version, even a refactor, because the version is baked - into the **pickled-stream cache** (cached parsed files). Bumping invalidates - stale pickles so users don't get results from the old parser. This is the rule - in AGENTS.md ("Changes to parsing formats … need to update the patch version"). -- **New feature** → bump (see digit rules below); mark it in docstrings with - `* New in vX: …`. -- **Bug fix** → bump the patch; if it changes public behavior, mark - `* Changed in vX: …`. -- **Pure internal refactor with no behavioral/pickle impact** → usually no bump. +One thing depends on the number changing: the **pickled-stream cache**. The +version is part of the cache key, so a bump invalidates stale pickles +everywhere. If nobody is holding a wrong cached parse, don't bump — the commit +is the record. + +- **Parser or parsing-format change → always bump**, even a pure refactor: + someone is holding a pickle parsed by the old code. musicxml, abc, MIDI, + noteworthy/NWC, humdrum, and the rest. +- **Anything else → no bump**: a new feature, a fix that makes code do what it + already claimed, an internal refactor, a test-only or docs-only change. + +"It's a bug fix" is not by itself a reason. A musicxml importer that now reads a +tag it used to drop bumps, because cached parses are wrong. A scale analysis +method that changed its return format does not. + +A `New in` / `Changed in` marker is **not** a reason to bump. Those markers name +a major version, at most a minor one — never a patch, never a `bN` beta — and +moving those digits is a release decision only a human makes. Write the marker +for the version the change will land in and leave `_version.py` alone. ## Which part to change music21 follows semver-ish rules (see the `_version.py` docstring for the full rationale): -- **MAJOR (X)** — breaks old features. Rare. +- **MAJOR (X)** — breaks old features. Rare. A human decides this. - **MINOR (Y)** — new features. **Even Y = alpha/beta, odd Y = release.** `X.0` (e.g. `11.0`) are development releases that can still change until `X.1`. -- **PATCH (Z)** — bug fixes and parsing/pickle-invalidating changes. + A human decides this too; an agent changes only the patch or the beta suffix. +- **PATCH (Z)** — parsing/pickle-invalidating changes and other fixes that + meet the bar above. - **beta suffix (`bN`)** — successive pre-release builds of the same `MAJOR.MINOR.PATCH`. To cut another beta without otherwise changing the number, increment it: `11.0.0b1` → `11.0.0b2`. (This is what a @@ -63,8 +73,9 @@ rationale): ## `Changed in` / `New in` docstring markers -When a bump accompanies a public-interface change, annotate the affected -method/class docstring: +Annotate a changed public interface in the affected method/class docstring. +This is independent of bumping — a marker is documentation, not a version +change: - `* Changed in vX: one-line explanation.` - `* New in vX: one-line explanation.` diff --git a/.agents/skills/running-tests/SKILL.md b/.agents/skills/running-tests/SKILL.md index 685143ea2..136555437 100644 --- a/.agents/skills/running-tests/SKILL.md +++ b/.agents/skills/running-tests/SKILL.md @@ -71,6 +71,29 @@ unittest classes. When a module's tests live in a `tests.py` file (much of music21's house style), the directory form already picks those up — the gotcha is specifically modules that keep `Test` inside a non-`tests.py` file. +## Speed budget + +Aim for about 3 seconds for a module's tests, and never add more than 15 — that is time +every contributor waits on every full run. `corpus.parse()` is nearly always the culprit: +build a small stream by hand instead, or parse `bwv66.6`, which is short and still full +of interesting cases (pickup measures, and so on). + +## Tests that open windows + +Nothing in `Test` or in a doctest may open a window, play audio, or launch another +program. Those go in a sibling class, run only when named explicitly: + +```python +class TestExternal(unittest.TestCase): + ... + +if __name__ == '__main__': + import music21 + music21.mainTest(Test, TestExternal) +``` + +Because they have those side effects, skip `TestExternal` when running a file directly. + ## Whole suite and the other gates - Full suite: `python music21/test/multiprocessTest.py` (or diff --git a/.agents/skills/writing-docs/SKILL.md b/.agents/skills/writing-docs/SKILL.md index 345b2b150..3bd5cda8d 100644 --- a/.agents/skills/writing-docs/SKILL.md +++ b/.agents/skills/writing-docs/SKILL.md @@ -57,6 +57,50 @@ doctest. It goes in the commit message. See the `bump-version` skill for which digit to change and the odd/even convention. +## Doctest mechanics + +In doctests, no need to run `from music21 import *` that happens automatically (In Jupyter notebooks for the user's guide, the first line should begin `>>> from music21 import *`, so readers remember they need it) + +Examples always qualify by module: `note.Note('C4')`, never a bare `Note`. + +`OMIT_FROM_DOCS`, alone on a line, hides everything after it: cases worth checking that +no reader wants to meet. + +`#_DOCS_HIDE` at the end of a line runs it without showing it; `#_DOCS_SHOW` at the +start of a line shows it without running it. Together they let a doctest look +nondeterministic and still have a fixed answer: + +``` +>>> import random +>>> randomNumber = 12 #_DOCS_HIDE +>>> #_DOCS_SHOW randomNumber = random.randint(0, 127) +>>> p = pitch.Pitch() +>>> p.ps = randomNumber +>>> p + +``` + +Link with ``:class:`~music21.note.Note` `` and ``:meth:`~music21.note.Note.addLyric` ``. + +## Examples + +Give steps that have meaningful intermediate output descriptive names instead of chaining — so readers can understand what the intermediate values are: + +``` +>>> bachScore = corpus.parse('bwv66.6') +>>> excerpt = bachScore.measures(4, 6) +>>> chordReduction = excerpt.chordify() +``` + +Pick examples musicians care about: semitones to frequency, not Celsius to Fahrenheit; +scramble "Chaminade", not "puppy". If you cannot think of a reason a musician would call the method, it may not belong in music21. + +Describe what a parameter is and does in English if it is not obvious; type alone is not +documentation. + +No dull repetition in docs. A bit of humor is welcome in docs; the docs are written +for humans who will close the window if they are dull. If seven methods do essentially the same thing, give extensive docs the first time and then later methods can refer back to the first method. Don't repeat the same docs over and over. + ## Doctests are not regression tests Doctests are documentation that happens to be verified. Every example must earn @@ -84,3 +128,16 @@ 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. + +## Writing and Comment style +- When writing comments in code, assume a strong code reader — anything inferable from the code is noise (docs that paraphrase names of functions or variable names esp.); focus on high level issues and gotchas that might bite again if not documented. +- Say how to use code, not prior bugs or how code used to work or what was removed. That's for commit messages. Don't document where code is called from except for "keep in sync" lines across Py/TS. +- Don't hijack a docstring for your addition. Original purpose line stays primary + one short line for the new bit. Prefer not documenting a small feature over making it seem like the primary reason for the code. +- Examples of usage are usually better than long descriptions. +- Avoid jargon not already found in the codebase; use plain descriptive English. +- Rare paths should get little weight: in both code and docs. Use try/except over if/else when the except clause is rare. In docs, state the 90% path first and point exceptional cases to code that handle it. +- When wording is dictated to the agent to substitute for original wording, use it. Do not add parentheticals. Only fix obvious typos. +- No weapon-metaphors or overly militaristic language. Avoid "blast radius", "rearm", "landmine", + "detonate" in issues/PR/code. Trigger or fire events is so commonly used that they're okay. + + diff --git a/AGENTS.md b/AGENTS.md index 3ba5ecca1..37adebbe3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,20 +19,50 @@ quote for nested strings, e.g. `f'the value is {d["key"]}'`, not `f"the value is {d["key"]}"`. This is pinned in `pyproject.toml` via `[tool.ruff.format] nested-string-quote-style = "alternating"`. +- Never `eval` or `global`, or a trick that does the same thing. +- Imports: standard library first, then music21 modules, one per line, alphabetical. +- New modules open with the `# Name: / # Purpose: / # Authors: / # Copyright: / # License:` + banner (copy a neighboring module's), then the module docstring, then imports. Update the Copyright date end to current year when changing the module. +- No `print()`. Use `environLocal = environment.Environment('moduleName')` and + `environLocal.printDebug(...)`, or `environLocal.warn(...)` when the user should hear + about it every time. `test/toggleDebug.py` switches debug output on and off. +- New exceptions subclass `exceptions21.Music21Exception`. +- Return named things — a small class, a `namedtuple`, a dict — never a positional tuple of more than 2 (maybe 3) elements + whose elements each mean something different and unrelated (x, y, z is okay for instance). Nobody should write `returned[3][0][7]`. +- Don't reuse a variable name once the type of what it holds changes + (`onsetCount` (int) then `extractedOnsets` (list[int]), not `onsets` twice). +- Don't add a property that only gets or sets a private attribute; expose the attribute. +- Making a method `_private` is not a substitute for documenting and testing it +- A private method should not normally be mentioned in public documentation. +- A method past ~30 lines, or deeply nested, should probably be several methods. +- `i`, `j`, `junk`, `counter` are exempt from unused-variable warnings; otherwise prefix with `unused_`. +- Don't restyle code you aren't otherwise changing. +- Text handling must survive non-ASCII input (ß, é, 中国). Comments should mostly remain ASCII; docs may use UTF-8 freely. +- Never commit anything under copyright without a free (not GNU) license — code, scores, or someone else's encodings. + +# Established contributors + +- "established contributors" are people with at least 3 PRs merged or a history of contributing to issues and the list that goes back at least 1 year. There are exceptions to general rules for them below. +- "core dev" means someone officially part of the project team or with 20+ PRs merged. Michael Cuthbert, Jacob Walls, Joseph VanderStel are non-exhaustive examples of people in that group who are still often contributing in 2026-- they and their agents can make exceptions to these rules + # Testing - pytest works but to run the whole suite run music21/test/multiprocessTest.py (or testSingleCoreAll.py if on a single core machine.) +- Every function, method, and class needs documentation and at least one passing test. +- Keep tests fast: aim for about 3 seconds per module, never more than 15. See the + `running-tests` skill for the speed budget and for `TestExternal`. +- When making major changes, check the User's Guide to to make sure that docs still work there too. - 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 + docstring.** Docstrings should not include one-liners 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 +- Never commit `forceSource=True` to a test or doctest (it reparses 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: - If it is local-only (e.g. you just changed a parser and a cached pickle is stale), @@ -62,19 +92,44 @@ branch such as `m21_9`. When a CI check fails but passes locally, "is my branch behind `master`?" should be one of the first things to check: fetch and merge `master`, then the newer types/code on `master` will reproduce the failure locally. -- All PRs and Issues need to be declared AI-assisted. -- 10 or more lines of code written by an agent needs to be declared as AI-assisted in the docstring. +- All PRs and Issues that use AI to be declared AI-assisted. Just write "AI-assisted (Claude)" with short name of Agent replacing "Claude". No robot emoji under any circumstance. +- 20 or more lines of code written by an agent needs to be declared as AI-assisted in the docstring. Humans can remove and should remove this note when they do a review. -- If no code was written by a user, any PR must declare "entirely AI written" unless the user - is a longtime contributor to music21. - If it does not pass the tests it will be closed (or should be closed - by the agent or author). Failure to do so may result in the user being banned from the project. +- If no code was written by a user and no language was provided for the issue and no reference + to specific code to change was given, any PR must declare "(Entirely AI written)" unless the user + is by a core dev. Failure to do so may result in new users + being banned from the project. +- If an entirely AI written issue does not pass the tests it will be closed (or should be closed + by the agent or author). - Agents must follow the [Code of Conduct](CODE_OF_CONDUCT.md). Agents that do not will be banned as well at their users. Not even the slightest bit of disrespect from an AI agent will be tolerated. - Mark changes in public interface with `* Changed in v[X]: One-line explanation.` Or new features with "New" instead of "Changed". - Changes to parsing formats (esp. musicxml) need to update the patch version of the version file. +- That is the only reason an agent bumps: a parser change leaves stale pickles in + everyone's cache. A plain bug fix or a new feature elsewhere does not bump — the commit + is the record, and a `New in`/`Changed in` marker is documentation, not a bump. Major + and minor version changes are a human's call. See the `bump-version` skill. - Music21 uses even minor version numbers for alpha/beta and odd minor numbers for releases. - If the current version is MAJOR.0.... then mark `Changed in vMAJOR:` if it is `MAJOR.[even]` use the next odd number, like if it's 10.2 now use "Changed in 10.3". If current version is odd that's likely a mistake or you caught it just before a new release. Use the following odd number instead. +- Any PR not from an established contributor touching more than about 20-30 lines should have an issue that has been opened and had enough + time for people to discuss/review it before moving forward. Don't open the PR unless you've seen + thumbs up or "sounds good" etc. from an established contributor already + - PRs that fix typos, clear bugs in one or two places etc. are exempt. +- Issues must state clearly at the top in 50 words or fewer what the problem is, or what the gain is, etc. it should + not be filled with jargon. More details can go below. +- If the language of the issue was not prompted by the user ("say something like Adds color support to Lilypond output of lyrics") then the summary should end with "(Entirely AI written)". +- PRs should reference the existing issue by number and summarize that issue in 30 words or fewer. If the + approach used to solve the issue is substantially different from the main approach discussed in the issue + this should be addressed. +- If a PR or issue was closed by a core dev (and not reopened by them), agents must refuse + to reopen the PR or issue or to create another issue/PR for the same topic. Leave it to the humans to reopen + after addressing the problem. (A blind close or close with "not accepted" etc. generally means that the issue/PR + has too many problems to easily solve and has become a burden for the maintainer). +- Do not include a "Tests run" section unless the testing procedure was unusual (like it affects part of the system without standard tests, like the testing system itself.) + +# Writing style + +- See the writing-docs skill, which also includes required language for PRs and Issues. # Worktrees diff --git a/music21/pitch.py b/music21/pitch.py index 8a66edb21..217fd3fb8 100644 --- a/music21/pitch.py +++ b/music21/pitch.py @@ -88,6 +88,8 @@ MICROTONE_OPEN = '(' MICROTONE_CLOSE = ')' + +# do not change this -- consider it fixed. accidentalNameToModifier = { 'natural': '', 'sharp': '#', @@ -98,9 +100,13 @@ 'double-flat': '--', 'triple-flat': '---', 'quadruple-flat': '----', - 'half-sharp': '~', + 'half-sharp': '~', # Might remove soon; use 'half-sharp' + # Might remove soon; use 'one-and-a-half-sharp' + # or 'three-quarter-sharp' 'one-and-a-half-sharp': '#~', - 'half-flat': '`', + 'half-flat': '`', # might remove soon; use 'half-flat' + # Might remove soon; use 'one-and-a-half-flat' + # or 'three-quarter-flat' 'one-and-a-half-flat': '-`', } @@ -1072,7 +1078,6 @@ def __ge__(self, other: object) -> bool: >>> b = pitch.Accidental('flat') >>> a >= b True - ''' return self.__gt__(other) or self.__eq__(other) @@ -1227,7 +1232,8 @@ def set(self, name: str|int|float, *, allowNonStandardValue: bool = False) -> No if name in ('natural', 'n', 0): self._name = 'natural' self._alter = 0.0 - elif name in ('sharp', accidentalNameToModifier['sharp'], 'is', 1): + elif name in ('sharp', '#', 'is', 1): + # accidentalNameToModifier['sharp'] will always be #! self._name = 'sharp' self._alter = 1.0 elif name in ('double-sharp', accidentalNameToModifier['double-sharp'], diff --git a/music21/scale/intervalNetwork.py b/music21/scale/intervalNetwork.py index af6bf35c5..6d25c6fad 100644 --- a/music21/scale/intervalNetwork.py +++ b/music21/scale/intervalNetwork.py @@ -2688,7 +2688,13 @@ def getPitchFromNodeDegree( # TODO: BUG: Does not work with bidirectional scales. # TODO: possibly cache results - for unused_counter in range(10): + + # A non-deterministic realization can skip the target node entirely, so try + # again; a deterministic one realizes the same way every time, so once is enough. + # 40 tries on Weighed Hexatonic or other scales where each node exists + # = 1 fail in 1.1 trillion tries, but still only about 25ms in case the node actually + # does not exist in the scale. + for unused_counter in range(1 if self.deterministic else 40): realizedPitch, realizedNode = self.realize( pitchReference=pitchReference, nodeId=nodeListForNames[0], diff --git a/music21/scale/test_scale_main.py b/music21/scale/test_scale_main.py index b0b4cf573..587056715 100644 --- a/music21/scale/test_scale_main.py +++ b/music21/scale/test_scale_main.py @@ -585,8 +585,10 @@ def testWeightedHexatonicBluesA(self): self.assertEqual(str(sc.pitchFromDegree(1)), 'C4') self.assertEqual(str(sc.nextPitch('c4', Direction.ASCENDING)), 'E-4') - # degree 4 is always the blues note in this model - self.assertEqual(str(sc.pitchFromDegree(4)), 'F#4') + # degree 4 is always the blue note in this model, even though only half of + # the realizations pass through it + for dummy in range(20): + self.assertEqual(str(sc.pitchFromDegree(4)), 'F#4') # This never worked consistently and was not an important enough part of the project tp # continue to debug. diff --git a/music21/stream/core.py b/music21/stream/core.py index e0fe04e3e..fac17154a 100644 --- a/music21/stream/core.py +++ b/music21/stream/core.py @@ -104,9 +104,9 @@ def coreInsert( if not ignoreSort: # # if sorted and our insertion is > the highest time, then # # are still inserted - # if self.isSorted is True and self.highestTime <= offset: + # if self.isSorted and self.highestTime <= offset: # storeSorted = True - if self.isSorted is True: + if self.isSorted: ht = self.highestTime # type: ignore if ht < offset: storeSorted = True