-
Notifications
You must be signed in to change notification settings - Fork 407
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
Romantext writer handle repeat bars and measure suffixes like m1a #1435
Romantext writer handle repeat bars and measure suffixes like m1a #1435
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing that must change is the typing on the def rnString()
-- mypy should be picking that up as an error. But there's the opportunity to make this much better than constantly reading from the same string.
@@ -332,11 +360,14 @@ def rnString(measureNumber: int, | |||
''' | |||
|
|||
if inString: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In definition of rnString, change type to t.Union[int, str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it looks like you're concating int(measure.number) + measure.suffix only to separate them later. why not pass in the suffix separately?
I didn't look carefully enough at rnString()
when this went in. It's quite inefficient. It seems to me that it really should be an object whose str gives the correct result so you don't need to keep passing in a the same string to concat to and read from over and over. But that's a later/larger refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all.
+1 for sorting out rnString
.
Mea culpa for how it looks as it stands though I suspect it ended up that way as one of those solutions that seeks to please everyone ... but ends up half way between different solutions and ends up pleasing no one ;)
See #1393 for an alternative that removes it completely.
I'm happy to advance this sometime.
@@ -332,11 +360,14 @@ def rnString(measureNumber: int, | |||
''' | |||
|
|||
if inString: | |||
inStringMeasureNumber = int(inString.split(' ')[0][1:]) | |||
if inStringMeasureNumber != measureNumber: | |||
inStringMeasureNumber = inString.split(' ')[0][1:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it looks like you're concating int(measure.number) + measure.suffix only to separate them later. why not pass in the suffix separately?
Do you mean on this line? The only thing I changed was removing the cast to int. It doesn't split the suffix, it splits the inString on space and then slices the "m" from the beginning. As far as I can tell the suffix stays put once it is added.
I agree this function is clearly inefficient. It seems the obvious thing to do would be accumulate each "word" to a list and then do ' '.join
when finished.
I fixed the type annotation. I notice that ironically the mypy run on github now fails, but it succeeds on my local machine, and it seems to be objecting to something in numpy. |
Seems like a known issue in mypy -- I'll do a patch to the github actions workflow for now. |
Patch is in -- rerunning tests |
While working on addressing #1412 I noticed that the RomanText writer is not writing out repeat bars or measure suffixes like "m1a".
I.e., previously if we read in
Then wrote it out, we would get
This PR fixes this issue by implementing writing of repeat bars and measure suffixes.