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

Parse double flats in RomanText #1405

Merged
merged 4 commits into from
Sep 4, 2022

Conversation

malcolmsailor
Copy link
Contributor

Fixes #1390.

For the vast majority of cases (where there is just a single flat symbol) the regex on line 83 is overkill so we only run that if the preceding conditions don't match.

@coveralls
Copy link

coveralls commented Sep 2, 2022

Coverage Status

Coverage increased (+0.0003%) to 93.066% when pulling 3847da9 on malcolmsailor:double-flats into ac6d6d2 on cuthbertLab:master.

music21/key.py Outdated Show resolved Hide resolved
music21/key.py Outdated Show resolved Hide resolved
Comment on lines +73 to +74
>>> key.convertKeyStringToMusic21KeyString('bbb')
'b--'
Copy link
Member

Choose a reason for hiding this comment

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

Also a test for a different double-flat key than b, say 'Ebb' since that follows a completely different code path potentially than 'bbb'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think both cases follow the same path since the checks for B/b/Bb/bb only apply when the key has two or fewer characters. But I added such a test.

I wasn't sure what the right balance between comprehensiveness and readability was here since these are actually doctests (albeit ones that people who aren't actually developing music21 are probably unlikely to consult).

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.

A few small changes to make the common case of Key of 'b' or 'B' work more quickly.

@malcolmsailor
Copy link
Contributor Author

OK, done. Rather than checking for 'B' explicitly I just pass if the last character is anything other than 'b'; then we don't check all those other redundant conditions if e.g., the key is "F#" or "E", etc.

@malcolmsailor
Copy link
Contributor Author

It just occurred to me that this function will throw an exception if textString is the empty string. Should we handle that case or is the exception ok?

@jacobtylerwalls jacobtylerwalls changed the title Double flats Parse double flats in RomanText Sep 3, 2022
@mscuthbert
Copy link
Member

Looks great! Thanks @malcolmsailor and for the last patch @jacobtylerwalls We can make empty string throw an exception later, but if so we should do it explicitly and not just with an IndexError which would be strange.

@mscuthbert mscuthbert merged commit 0e5b173 into cuthbertLab:master Sep 4, 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.

Parsing double-flat keys in RomanText
3 participants