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

fixed up romanText repeats #1378

Merged
merged 3 commits into from
Aug 15, 2022
Merged

Conversation

malcolmsailor
Copy link
Contributor

This PR addresses much of #858, though not (I think) the case of mid-measure repeats.

It turns out that romanText repeats were already largely implemented (by whoever wrote the romanText/translate.py, presumably), but there were a couple bugs that were preventing it from working as expected and there was no test.

I wrote some test cases and debugged the existing code. The bugs are explained in comments in the code. As far as I can tell, the repeats now work smoothly.

I also have a few (non-urgent, I think) questions in the comments. I'll repeat them here:

  1. repeat.Expander does not work on stream.Stream, only on stream.Part. Is this expected behavior?
  2. in the case of 1st and 2nd endings, spanner.RepeatBracket.getNumberList does not return the measure numbers I expect. What is the correct approach here?

@malcolmsailor
Copy link
Contributor Author

pylint objects to a bunch of stuff in translate.py but I don't believe any of it is introduced by this PR

@coveralls
Copy link

coveralls commented Aug 14, 2022

Coverage Status

Coverage increased (+0.01%) to 93.034% when pulling f8f72a4 on malcolmsailor:roman_repeats into 1a0ec50 on cuthbertLab:master.

@mscuthbert
Copy link
Member

yes, the problems were introduced in this PR when additional modules were imported at the top of the file. romanText.translate cannot import converter, because romanText is itself a converter, so we'd get a circular import.

@malcolmsailor
Copy link
Contributor Author

Oops, sorry about that, right you are. It slipped my mind that I had imported converter. I should have been more careful and linted the code without my changes.

Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

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

I'm going to merge this, since three of the issues are small (style/grammar) changes in test code, and the one substantive one (the TODO) is described carefully and can be a separate issue. I'm concerned that getNumberList() is returning false information, but that seems like either a misunderstanding of all of us, or a separate bug.

music21/romanText/translate.py Show resolved Hide resolved
music21/romanText/translate.py Show resolved Hide resolved
music21/romanText/translate.py Show resolved Hide resolved
music21/romanText/translate.py Show resolved Hide resolved
@mscuthbert mscuthbert merged commit 48deadd into cuthbertLab:master Aug 15, 2022
@mscuthbert mscuthbert mentioned this pull request Aug 15, 2022
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.

3 participants