Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1561: Let makeTies iterate over parts #1571

Merged
merged 5 commits into from
Jul 2, 2023

Conversation

TimFelixBeyer
Copy link
Contributor

@TimFelixBeyer TimFelixBeyer commented May 15, 2023

Fixes #1561

Allows makeTies to iterate over substreams and includes a regression test.
Let me know if there is anything you'd like to see addressed!

Allows makeTies to iterate over substreams and includes a regression test.
@coveralls
Copy link

coveralls commented May 15, 2023

Coverage Status

coverage: 93.03% (+0.003%) from 93.027% when pulling e9c87a5 on TimFelixBeyer:patch-5 into 9951a8e on cuthbertLab:master.

@mscuthbert
Copy link
Member

I think it would be better if makeTies were overloaded in Score and Opus -- I prefer superclasses not to need to think about subclass behavior (as much as possible -- there are already exceptions in the codebase, but I'm trying to remove them rather than add more). It may be that Score, Part, Measure, etc. someday are moved to a different file from stream.Stream.

@mscuthbert
Copy link
Member

Hello -- just as a note (from the music21list Google Group) I'm taking a sabbatical from reviewing music21 issues and PRs until at least Jan 1, 2024, so this PR will be deferred until then.

@mscuthbert
Copy link
Member

Turns out that it's not possible to overload this easily within Score/Opus, so it's fine as is for those parts.

However, the newly added if returnObj.hasVoices() part was untested and the code below already took into account streams with voices. I'm removing it for now. The Coveralls test results will show you which lines of code have been added or edited and are not yet tested. I added tests for Score/Opus, but until Voices is tested cannot add that.

Screenshot 2023-07-01 at 14 42 25

I'm merging this once all tests pass and then will consider requests again after sabbatical.

@mscuthbert mscuthbert merged commit 3592086 into cuthbertLab:master Jul 2, 2023
@TimFelixBeyer TimFelixBeyer deleted the patch-5 branch July 2, 2023 01:40
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.

Let makeTies() iterate over parts
3 participants