Ignore trailing text after a Scala pitch value - #2026
Merged
Conversation
ScalaData.parse skips a line only when it starts with '!', so a degree line carrying a comment after the value is handed to ScalaPitch, whose _setSrc keeps every digit, '.' and '/' from anywhere in the string. The comment's digits are spliced onto the end of the number rather than dropped, so `10/9 ! A\ ; 8 |mi|la|re|` is read as '10/98' -- -3951.34 cents where 10/9 is 182.40, and no exception is raised. The Scala format ignores any characters following the pitch value, which is why the bundled files write comments there. Take the first whitespace-delimited token on the pitch-count line and on each degree line. 23 of the 3932 files in scale/scala/scl/ were misread. 21 raised ValueError, including every file whose comment contributes a second '/'; 2 more parsed without error and gave wrong cents, newton_15_out_of_53 and sparschuh-jsbloops440. Trailing text without a '!' took the same path: rvf1, rvf2 and rvf3 write a degree as `454.75 (427)`. All of them read correctly now, and no other file's parsed values change. Two tests. testCommentAfterAPitchValue covers a '!' comment, bare trailing text, and the two bundled files that used to be read silently wrong, pinning their cents. testEveryBundledScalaFileParses reads the whole archive and asserts every file declares a pitch count, yields that many values, and gives a finite cents for each -- so a file added or edited upstream that this parser cannot read fails there rather than wherever some scale is later asked for. It takes about 0.8s. The description line is not checked, since the format allows it to be empty.
Member
|
Thank you! Wonderful PR and big help. My only concern originally was whether testEveryBundledScalaFileParses() was going to take too long to make it run every time we do the test suite, but 100ms is definitely fast enough to be worth avoiding this kind of bug in the future. [14]: %timeit t.testEveryBundledScalaFileParses()
105 ms ± 775 μs per loop (mean ± std. dev. of 7 runs, 10 loops each) |
Contributor
Author
|
Happy to help (: |
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
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.
ScalaData.parse skips a line only when it starts with '!', so a degree line carrying a comment after the value is handed to ScalaPitch, whose _setSrc keeps every digit, '.' and '/' from anywhere in the string. The comment's digits are spliced onto the end of the number rather than dropped, so
10/9 ! A\ ; 8 |mi|la|re|is read as '10/98' -- -3951.34 cents where 10/9 is 182.40, and no exception is raised.The Scala format ignores any characters following the pitch value, which is why the bundled files write comments there. Take the first whitespace-delimited token on the pitch-count line and on each degree line.
23 of the 3932 files in scale/scala/scl/ were misread. 21 raised ValueError, including every file whose comment contributes a second '/'; 2 more parsed without error and gave wrong cents, newton_15_out_of_53 and sparschuh-jsbloops440. Trailing text without a '!' took the same path: rvf1, rvf2 and rvf3 write a degree as
454.75 (427). All of them read correctly now, and no other file's parsed values change.Two tests. testCommentAfterAPitchValue covers a '!' comment, bare trailing text, and the two bundled files that used to be read silently wrong, pinning their cents. testEveryBundledScalaFileParses reads the whole archive and asserts every file declares a pitch count, yields that many values, and gives a finite cents for each -- so a file added or edited upstream that this parser cannot read fails there rather than wherever some scale is later asked for. It takes about 0.8s. The description line is not checked, since the format allows it to be empty.