feat(song): per-note keys hand assignment (hand) on the Note wire#990
Open
ChrisBeWithYou wants to merge 1 commit into
Open
feat(song): per-note keys hand assignment (hand) on the Note wire#990ChrisBeWithYou wants to merge 1 commit into
hand) on the Note wire#990ChrisBeWithYou wants to merge 1 commit into
Conversation
The editor's keys LH/RH hand arc needs a per-note hand assignment
('lh'/'rh', from a MusicXML grand staff import today, hand-editable
later) to survive a sloppak save → reload: the editor already emits it
on the wire, but note_from_wire dropped unknown keys, so the field died
on every reopen.
- Note gains `hand: str | None = None` (None = unassigned; the
heuristic hand split keeps owning unassigned notes). Distinct from
`right_hand` (the bass plucking finger) — hence the spelled-out
`hand` wire key, since `rh` is taken.
- note_to_wire emits it default-omitted and validates on emit; older
readers ignore it (feedpak: unknown note keys are permitted).
- note_from_wire decodes it as a strict enum — anything but 'lh'/'rh'
(junk, wrong case, bools) falls back to unassigned rather than
poisoning downstream hand-split / hands-separate practice logic.
Groundwork consumers land separately: notation_lift.split_hands
respecting per-note overrides, and the editor's hand surface.
Editor counterpart: feedBack-plugin-editor #299.
Tests: three new wire round-trip tests in tests/test_song.py (literal
key, default-omitted, junk rejection both directions), matching the
teaching-marks test style.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesNote hand assignment
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
What
Adds a per-note keys hand assignment to the Note model and wire:
hand: 'lh' | 'rh' | None(None = unassigned — the heuristic hand split keeps owning unassigned notes).handkey —rhis taken by right_hand (the bass plucking finger).'lh'/'rh'(junk, wrong case, bools) decodes to unassigned rather than poisoning downstream hand-split / hands-separate-practice logic.additionalProperties: true).Why
The editor's keys LH/RH hand-assignment arc: MusicXML grand-staff imports carry authored per-note hands (editor PR feedBack-plugin-editor#299 + musicxml-import#8), and the editor emits the field on its save wire — but core's
note_from_wiredrops unknown keys, so the assignment died on every sloppak reopen. This makes the round-trip real.Follow-ups land separately:
notation_lift.split_handsrespecting per-note overrides (so edited bars keep authored hands), and the editor's hand surface (roll shading, split-point stamping, hands-separate practice).Tests
Three new wire round-trip tests in
tests/test_song.py(literal key pinned, default-omitted, junk rejected both directions), matching the teaching-marks test style. Suite: 1721 passed locally (+3 vs main; the 99 pre-existing failures/errors reproduce identically on pristine main — local env, missingmidoetc.).🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Bug Fixes