Skip to content

feat(core): parse GP let-ring into a LetRing technique + tag (#75, part 1) - #87

Merged
PhysShell merged 3 commits into
mainfrom
claude/modest-maxwell-0zecwn
Jun 19, 2026
Merged

feat(core): parse GP let-ring into a LetRing technique + tag (#75, part 1)#87
PhysShell merged 3 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 19, 2026

Copy link
Copy Markdown
Owner

First slice of #75 (auto-derive tags/techniques). #75 is really four independent derivations — technique parsing, syncopated, chord quality, structure — and this starts the technique slice with let-ring, the one that genuinely needs a new model variant (pull-off and tap already have their variants; only their emission is missing, and both need caller/sequence context, so they follow separately).

What

Adds let-ring end-to-end, mirroring how palm_mute is handled:

  • Model event.rs: new SpanTechnique::LetRing.
  • Tag corpus.rs: new SwancoreTag::LetRing (+ all_variants). SwancoreTag is #[serde(rename_all="snake_case")], so it serializes as "let_ring" and joins the web tag palette automatically.
  • Import gp.rs: map_gp_note_marks maps note.effect.let_ring → an explicit LetRing span.
  • Derivation technique.rs: LetRing → name "let_ring" + tag SwancoreTag::LetRing.
  • Exhaustive SpanTechnique matches updated in complement.rs and dump.rs.

Approach

Red → green: 70db92f adds the failing gp.rs test (references SpanTechnique::LetRing, compile-fail red); 029c0f4 adds the variant through the stack + a derivation test.

Verification

  • core suite green (new: gp_let_ring_emits_explicit_span, and let_ring_span_derives_the_let_ring_tag_and_name — a let-ring note → let_ring tag + name); cli green; clippy -D warnings clean; web crate compiles (the new tag rippled cleanly).

Remaining #75 technique slice

  • pull-offSpanTechnique::PullOff/SwancoreTag::PullOff already exist; gp.rs must emit PullOff vs HammerOn by direction (the previous same-string pitch, available via acc.held). Its own PR.
  • tapNoteMark::Tap exists; set it from the beat's slap_effect == Tapping. Its own PR.

Then the other three #75 derivations (syncopated, chords, structure).

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • New Features
    • Added support for the Let Ring spanning technique, enabling sustained-note labeling carried into following beats and surfacing it in technique overlap analysis.
    • Guitar Pro imports now recognize let_ring note effects and include them in derived technique spans and tags.
  • Tests
    • Added coverage to confirm Let Ring mapping and correct derived tag/name output.
  • Documentation
    • Updated tagging taxonomy guidance for forward-compatible, additively growing enums and schema stability.

claude added 2 commits June 19, 2026 15:10
Failing test for map_gp_note_marks: a note with effect.let_ring should emit an
explicit SpanTechnique::LetRing (a new, missing technique variant). The variant
doesn't exist yet, so this fails to compile until green.
…75)

Adds SpanTechnique::LetRing and SwancoreTag::LetRing end-to-end: gp.rs maps
note.effect.let_ring to an explicit LetRing span; technique.rs derives it to the
"let_ring" name and the SwancoreTag::LetRing tag (like palm_mute); span labels
added in complement.rs/dump.rs; the tag joins all_variants (serde snake_case →
"let_ring", so it's in the web tag palette automatically).

First of the #75 technique-parsing slice. core suite green; cli green; clippy
clean; web compiles. pull-off (HO/PO direction) and tap (beat-level) follow.
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d60317e-54b7-420f-8899-cbed5d59045e

📥 Commits

Reviewing files that changed from the base of the PR and between 029c0f4 and 333a3cc.

📒 Files selected for processing (2)
  • core/src/corpus.rs
  • docs/decisions.log.md
✅ Files skipped from review due to trivial changes (1)
  • docs/decisions.log.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/src/corpus.rs

📝 Walkthrough

Walkthrough

Adds SpanTechnique::LetRing end-to-end: the variant is declared in the core event enum, registered as SwancoreTag::LetRing in the corpus, mapped from Guitar Pro's let_ring note effect flag, wired into technique derivation, serialization (span_name), and the complement axis label, with unit tests covering both the GP importer and the derivation pipeline.

Changes

LetRing Technique Pipeline

Layer / File(s) Summary
SpanTechnique and SwancoreTag enum additions
core/src/event.rs, core/src/corpus.rs
SpanTechnique::LetRing added to the core spanning-technique enum with doc comment; SwancoreTag::LetRing added to the corpus enum with doc comment and appended to all_variants() after PalmMute; corpus module documentation updated to clarify the tag taxonomy is not schema-versioned and grows additively.
Guitar Pro importer mapping
core/src/gp.rs
map_gp_note_marks now checks GpNoteEffect::let_ring and emits a TechniqueSpan with SpanTechnique::LetRing and explicit evidence; a new unit test (gp_let_ring_emits_explicit_span) asserts the mapping produces exactly one such span.
Derivation, serialization, and complement axis
core/src/technique.rs, core/src/dump.rs, core/src/complement.rs, core/tests/technique_tags.rs
SPANS list, technique-to-name, and technique-to-tag helpers updated to recognize LetRing; span_name and span_label both map to "let_ring"; a technique_tags test asserts SwancoreTag::LetRing and "let_ring" appear in derived output.
Schema compatibility decision
docs/decisions.log.md
Appends a 2026-06-19 entry documenting the strategy: keep SCHEMA_VERSION = 7, grow the tag taxonomy additively, and rely on forward-compatible optional-field patterns so older schema-7 tooling correctly hard-rejects chunks with unknown enum variants.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

  • PhysShell/griff#51: Modifies the same map_gp_note_marks function in core/src/gp.rs to emit technique spans for other note effects (dead notes, Bend), using the same pattern this PR applies for LetRing.
  • PhysShell/griff#69: Modifies the same spanning-technique derivation pipeline in core/src/technique.rs — the SPANS list, technique-to-name, and technique-to-tag maps — following the identical extension pattern used here for LetRing.

Poem

🐇 Hop hop, I let the string ring free,
A LetRing variant, neat as can be!
From Guitar Pro flags to corpus tags,
Through spans and labels, nothing lags.
The enum grows, the tests all pass —
This rabbit's code is built to last! 🎸

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: implementing let-ring parsing from Guitar Pro files as a new SpanTechnique and SwancoreTag. It directly corresponds to the changeset's primary objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/modest-maxwell-0zecwn

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 029c0f4423

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread core/src/corpus.rs
Vibrato,
PalmMute,
/// Let ring — notes left to sustain into following beats.
LetRing,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bump the corpus schema for the new tag

Adding SwancoreTag::LetRing creates a new serialized corpus tag value ("let_ring"), and all_variants() now exposes it to the CLI/web curation paths that write ChunkMeta.tags, but SCHEMA_VERSION remains 7. A newly curated v7 manifest or chunk containing this tag is no longer readable by older v7 tooling because serde rejects unknown enum variants, so the version no longer identifies compatible corpus data; bump the schema version and document the tag-taxonomy change when adding this persisted enum value.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is right (serde rejects unknown variants), but we're deliberately keeping SCHEMA_VERSION = 7 — maintainer's call. Every version v1–v7 is a structural ChunkMeta change (a new optional field under the forward-compatible "pre-vN records keep loading" pattern); none tracks the tag set. A SwancoreTag addition fits neither that mechanism nor that compat direction (yours is backward compat — old reader, new data), and #75 adds ~15 more tags, so versioning the taxonomy would turn SCHEMA_VERSION into a tag counter and muddy what it identifies.

Made the policy explicit so it isn't ambiguous going forward (333a3cc): a note in the SCHEMA_VERSION doc + a decisions-log Y-statement. The residual risk (a pinned pre-tag build hard-rejecting a chunk with a newer tag) is accepted — griff's reader and writer ship together.


Generated by Claude Code

Per the maintainer's call on #87: SCHEMA_VERSION stays 7 for additive SwancoreTag
variants (let_ring, #75). The version tracks structural ChunkMeta field additions
(the v1–v7 optional-field, forward-compatible pattern), not the tag set — so a
new tag isn't a version event. Documented in the SCHEMA_VERSION doc + a
decisions-log Y-statement so it isn't re-flagged as #75 adds ~15 more tags.
@PhysShell
PhysShell merged commit 1fcf816 into main Jun 19, 2026
1 check passed
PhysShell pushed a commit that referenced this pull request Jun 19, 2026
The browser/phone capture path (`build_chunk_meta_record`) derived and
merged only technique tags, so a GP chord the CLI now serializes with
`maj7`/`power_chord` was emitted without those harmony tags — making corpus
tags depend on which curation front produced the chunk (Codex P2 on #88).
Mirror the CLI: derive harmony tags and merge them additively, with a
parity test.

Also fixes the adjacent technique-parity test, whose hardcoded `tags_idx`
"21" no longer selected Intro: #87's LetRing shifted Intro to index 22 in
`all_variants()` (web host tests aren't CI-gated, so it went unnoticed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants