docs: confirm isTieStart+isTieStop already model the continuation tie#328
Open
webern wants to merge 1 commit into
Open
docs: confirm isTieStart+isTieStop already model the continuation tie#328webern wants to merge 1 commit into
webern wants to merge 1 commit into
Conversation
webern
added
feature
Claude
6 tasks
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.8% | 6391 / 7913 |
| Functions | 67.7% | 2217 / 3275 |
| Branches | 49.5% | 5476 / 11052 |
Core HTML report | API HTML report
Commit 374a7599d101ee30eefafd5c8dfff56f4c54fdfa.
gen-quality
|
#291 asked whether mx::api needs a dedicated ContinuationTie class (by analogy with TieLetRing, #286) to model "ties at the start of 2nd endings and the like." Per the MusicXML spec (core::TiedType's doc comment): "For other ties that are visually attached to a single note, such as a tie leading into or out of a repeated section or coda, use two tied elements on the same note, one start and one stop." NoteData::isTieStart/isTieStop are already independent bools (unlike TieLetRing's lone, unpaired mark, this is an ordinary tie on each side), and NoteWriter already emits both <tie> and <tied> pairs when both are set (NoteWriter::addTie, called for stop then start). So the exact scenario rpatters1 described already round-trips with the existing model -- a new class would just be a second, redundant way to say the same thing, which is what #249's "one fact, one field" is explicitly meant to prevent. Added a regression test proving it (a note with isTieStop and isTieStart both true survives write -> read intact) and expanded the field's doc comment to state this directly, since the shape isn't obvious from the field declarations alone -- closing the loop on the design question with a concrete, working answer instead of new API surface. ## Testing - [x] New `tieStopAndStartOnSameNote` test (NoteDataTest.cpp) - [x] make test: all pass (4734 assertions in 379 test cases, plus examples) - [x] make test-api-roundtrip: 158 passed, 0 failed (of 158 pinned; no regressions -- no api behavior changed) - [x] make fmt / make check: clean Closes #291
webern
force-pushed
the
support-continuation-tie
branch
from
c20593c to
f4e6166
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.9% | 6424 / 7943 |
| Functions | 67.8% | 2221 / 3276 |
| Branches | 49.6% | 5504 / 11100 |
Core HTML report | API HTML report
Commit 9a83f0e2b7d027307ba571581c93251d4a57a8b8.
gen-quality
|
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.
Human Summary
Interesting. Instead of implementing a continuation tie for #291, the agent confirmed that it is not technically needed and wrote documentation to that effect.
Summary
#291 asked whether
mx::apineeds a dedicatedContinuationTieclass (by analogy withTieLetRing, #286) to model "ties at the start of 2nd endings and the like."Per the MusicXML spec (
core::TiedType's doc comment): "For other ties that are visually attachedto a single note, such as a tie leading into or out of a repeated section or coda, use two tied
elements on the same note, one start and one stop."
NoteData::isTieStart/isTieStopare alreadyindependent bools (unlike
TieLetRing's lone, unpaired mark, this is an ordinary tie on each side),and
NoteWriteralready emits both<tie>and<tied>pairs when both are set(
NoteWriter::addTie, called for stop then start). So the exact scenario rpatters1 describedalready round-trips with the existing model -- a new class would just be a second, redundant way to
say the same thing, which is what #249's "one fact, one field" principle is explicitly meant to
prevent.
Added a regression test proving it (a note with
isTieStopandisTieStartboth true surviveswrite -> read intact) and expanded the field's doc comment to state this directly, since the shape
isn't obvious from the field declarations alone -- closing the loop on the design question with a
concrete, working answer instead of new API surface.
Testing
tieStopAndStartOnSameNotetest (NoteDataTest.cpp)make test: all pass (4734 assertions in 379 test cases, plus the three examples)make test-api-roundtrip: 158 passed, 0 failed (of 158 pinned; no regressions -- no apibehavior changed)
make fmt/make check: cleanReferences
TieLetRing, the one tie shape that genuinely doesn't fitstart/stop pairing), api: replace value +
isSpecifiedbool pairs with an explicit optional value type #249 (one fact, one field)